Snowflake-Labs / terraform-provider-snowflake

Terraform provider for managing Snowflake accounts
https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest
MIT License
555 stars 422 forks source link

adding array assignment to snowflake_grant_account_role and snowflake_grant_privileges_to_account_role #2668

Open bob-zarkoob opened 8 months ago

bob-zarkoob commented 8 months ago

Terraform CLI and Provider Versions

Provider: 0.87.2 CLI: 1.7.2

Use Cases or Problem Statement

after you added these 2 resources, now we cannot use arrays to create the hierarchy between users and roles, and between roles and roles, now we have created a new resource for each assignment.

Like with snowflake_role_grants we used to have:

resource "snowflake_grant_account_role" "prod_bronze_read_arl" {
  role_name = snowflake_role.prod_bronze_read_arl.name

  roles = [
    snowflake_role.data_analyst_frl.name,
    snowflake_role.tableau_developer_frl.name,
    snowflake_role.data_scientist_frl.name
  ]

and now we to create 3 different resource like this: resource "snowflake_grant_account_role" "prod_bronze_read_arl_1" {

  role_name        = snowflake_role.prod_bronze_read_arl.name
  parent_role_name = snowflake_role.data_analyst_frl.name
}

resource "snowflake_grant_account_role" "prod_bronze_read_arl_2" {
  role_name        = snowflake_role.prod_bronze_read_arl.name
  parent_role_name = snowflake_role.tableau_developer_frl.name
}

resource "snowflake_grant_account_role" "prod_bronze_read_arl_3" {
  role_name = snowflake_role.prod_bronze_read_arl.name
  parent_role_name = snowflake_role.data_scientist_frl.name
}

Proposal

I like to be able to add users and roles in bulk using arrays. for snowflake_grant_privileges_to_account_role, I like to be able to add more than one object when granting privileges, like instead of doing 3 different resources to grant select privilege to 3 different objects, having the capability of adding multiple privileges to multiple roles.

How much impact is this issue causing?

Medium

Additional Information

No response

sfc-gh-jcieslak commented 8 months ago

Hey @bob-zarkoob πŸ‘‹ We'll consider adding "multiple grants by one resource", but for now, try to use builtin HCL for_each meta-argument to minimize the verbosity.

Bryan-Meier commented 7 months ago

I agree with @bob-zarkoob, we are now forced to use in some cases dozens of more resources in a for_each to create something that used to only take 1. This doesn't seem like a big deal but for those that are on Terraform Cloud (and haven't moved off πŸ˜„) every resource is more money.

gthomson31 commented 7 months ago

_foreach works but is flagging usernames as sensitive in the new resource which was not the case previously as a result permissions are being added with index instead which is a workaround.

What was the reason for the change ?


locals {
  role_user_acccess = [
    snowflake_user.example_user.name,
    snowflake_user.example_user2.name,
    snowflake_user.example_user3.name,
  ]
}

# --- User + Grants

resource "snowflake_grant_account_role" "example_user_role_grant" {
  for_each = { for idx, name in local.role_user_acccess : idx => name }
  role_name = snowflake_role.exmaple_role.name
  user_name = each.value
}
sfc-gh-jcieslak commented 7 months ago

Hey @gthomson31 Not sure what you mean here, for_each doesn't make users' name sensitive. It's already marked as sensitive in the user's schema (it's marked in the documentation).

gthomson31 commented 7 months ago

Interesting as this problem has only appeared snowflake_grant_privileges_to_account_role resource type this issue has not occured on previous grant types

sfc-gh-jcieslak commented 7 months ago

@gthomson31 Still not sure what is the issue in your case.

as a result permissions are being added with index instead which is a workaround

What does it mean permissions are being added with an index (and that it's a workaround) and in what sense the sensitive usernames are causing the issue?

I would be grateful if you could provide some examples with a brief description.

sfc-gh-jcieslak commented 7 months ago

@bob-zarkoob @Bryan-Meier We have it somewhere on our very long list of things to do and it's certainly an important topic, but unfortunately, It probably will be addressed not that soon as we're moving into other things that need to be done to finally achieve the v1.0.0 version. The next topics we'll be looking into are described here.

Bryan-Meier commented 7 months ago

@sfc-gh-jcieslak, I see what @gthomson31 was referring to with regard to sensitive values. I could be wrong here but this feels like the error is being generated by Terraform and not the provider. The issue is that in order to replace an old resource that with something like "snowflake_grant_account_role" while using a for_each where users are being iterated over, it seems to violate some terraform rule. Again, I could be wrong on that...

This is the error I am getting which is probably the same or similar to what @gthomson31 was reporting: image

This is the code it's referring to: image

This was the code that used the deprecated resource which works fine: image

Without some sort of workaround. I am stuck and not able to move from the deprecated "snowflake_role_grants" to the replacement "snowflake_grant_account_role".

Bryan-Meier commented 7 months ago

If there isn't a work around for this, it feels like this is more of a bug than a feature request.

gthomson31 commented 7 months ago

Yeah @Bryan-Meier exactly that have been trying to replace all our grants with the new method of assign users against a role although previously we used lists of users directly within the role grant but as the new resource doesn't support this then using for_each was the workaround which then brought up the problem of having to use indexing as the username can't be used.

Wasn't aware it was possible to do in the past but your comment highlighted it was

resource "snowflake_role_grants" "dev_role_grants" {

  role_name = snowflake_role.dev_role.name

  users = [
    snowflake_user.example_user.name,

snowflake_user.example2_user.name,
  snowflake_user.example3_user.name
]
}
sfc-gh-jcieslak commented 7 months ago

@gthomson31 @Bryan-Meier I see that sensitive is put on user name and not on login_name which is very weird. Anyway, we'll be reworking resources, because ones like user weren't changed for a long time. A workaround for sensitive + for_each could be to wrap "sensitive" values in the nonsensitive function.

Check the example below

resource "snowflake_user" "test" {
  name = "test_user_test_test"
}

resource "snowflake_role" "test" {
  name = "test_role"
}

resource "snowflake_grant_account_role" "test" {
  for_each = toset([for u in [snowflake_user.test.name] : nonsensitive(u)])
  role_name = snowflake_role.test.name
  user_name = each.value
}
Bryan-Meier commented 7 months ago

@sfc-gh-jcieslak, thanks for the response and I'm glad we got down to the bottom of the issue. I had no idea that there was such a function of nonsensitive. That should fix my issue while the resources are being reworked. I appreciate you passing that function my way!!

@gthomson31, I am guessing the nonsensitive function helps in your situation also.

Bryan-Meier commented 7 months ago

@sfc-gh-jcieslak, from what I can tell your suggestion of using the nonsensitive function doesn't work in that it throws other errors. This seems to be tied to a ton of questions throughout the community. Here is one example: https://github.com/hashicorp/terraform/issues/28222

I have tried applying the nonsensitive function in the for_each and was left with this message:

β”‚ Error: Invalid function argument
β”‚ 
β”‚   on modules/snowflake_role/main.tf line 22, in resource "snowflake_grant_account_role" "grant_role_to_users":
β”‚   22:   for_each = toset([ for u in var.role_users : nonsensitive(u) ])
β”‚ 
β”‚ Invalid value for "value" parameter: the given value is not sensitive, so
β”‚ this call is redundant. 

This is telling me it's nonsensitive but when I remove the nonsensitive function I get:

β”‚ Error: Invalid for_each argument
β”‚ 
β”‚   on modules/snowflake_role/main.tf line 22, in resource "snowflake_grant_account_role" "grant_role_to_users":
β”‚   22:   for_each = toset(var.role_users)
β”‚     β”œβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€
β”‚     β”‚ var.role_users has a sensitive value
β”‚ 
β”‚ Sensitive values, or values derived from sensitive values, cannot be used
β”‚ as for_each arguments. If used, the sensitive value could be exposed as a
β”‚ resource instance key.

I have even tried tricking it where I wrap sensitive function with a nonsensitive function which was also unsuccessful: for_each = toset([ for u in var.role_users : nonsensitive(sensitive(u.name)) ])

I have tried applying these techniques to the resources within the module I am working with as wells as the within the main caller module which is passing in the variable values (in a nonsensitve form).

I'm not sure what else to try.

Bryan-Meier commented 7 months ago

I tried one more thing before I gave up and it seemed to work. It's definitely not the preferred method but it does work.

Instead of trying to convert the name from a snowflake_user resource (i.e. for_each = toset([for u in [snowflake_user.test.name] : nonsensitive(u)])), I passed in a list of email addresses for the users. Since it's nothing more than a list of strings it runs without needing the nonsensitive function.

Again, not what we want to do but it does work.

sfc-gh-jcieslak commented 7 months ago

Yeah, the function is not perfect and I also got the errors you mentioned, but in the end, I got it working with the example I put in my last comment. We will surely work on a proper solution, but for now, let's treat the nonsensitive function as a workaround.

natashamathur commented 7 months ago

Thank you! Will the function be changed before the current granting method is deprecated? I am trying to decide whether to migrate over all of our users grants right now or wait.

natashamathur commented 7 months ago

HI! @sfc-gh-jcieslak There's a note here about plans to rework users. Do you have a timeline for roughly when that'll be happening?

sfc-gh-jcieslak commented 7 months ago

Hey @natashamathur

  1. Not sure what function you're referring to, but nonsensitive is the HCL function, it's not provided by us. It's good to migrate sooner than later, because of this.
  2. I can't give the exact timeline, but it should happen this quarter.
natashamathur commented 7 months ago

Got it! Your comment further upthread suggested that the users assignment itself will be further changed. Is that the plan?

sfc-gh-jcieslak commented 7 months ago

It's more about the sensitivity of the fields in the user resource rather than the assignment itself, but yeah, we'll take a closer look at what should be a sensitive value and what shouldn't when going over the user resource when working on the GA feature objects. Highly suggest going over our latest entry in the ROADMAP that describes our priorities in the next months. Especially this part where we linked a lists of objects we'll be refactoring this and the next quarter.

natashamathur commented 7 months ago

Thank you!

On Tue, May 7, 2024 at 8:09β€―AM Jan CieΕ›lak @.***> wrote:

It's more about the sensitivity of the fields in the user resource rather than the assignment itself, but yeah, we'll take a closer look at what should be a sensitive value and what shouldn't when going over GA feature objects. Highly suggest going over our latest entry in the ROADMAP https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md that describes our priorities in the next months. Especially this https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#preparing-essential-ga-objects-for-the-provider-v1 part where we linked a lists of objects we'll be refactoring this and the next quarter.

β€” Reply to this email directly, view it on GitHub https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2668#issuecomment-2098256529, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIO2SKYQZAT5EGEG6NR4S6LZBC753AVCNFSM6AAAAABFTY4AEWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJYGI2TMNJSHE . You are receiving this because you were mentioned.Message ID: @.*** .com>

--

Natasha Mathur

Director of Data

Calendly https://calendly.com/natashamathur/30-minute-meeting

Technology Advancing Primary Care

bob-zarkoob commented 4 months ago

@sfc-gh-jcieslak πŸ‘‹, do you have any ETA on when this issue can be worked on? The problem is we are paying for Terraform Cloud for each resource and the way this resource works, we ended up with thousands of resources and significantly higher cost for TFC.

sfc-gh-jcieslak commented 4 months ago

Hey @bob-zarkoob Unfortunately, right now our focus is on different topics (currently) as we're preparing resources for the V1 while addressing other big issues in the provider (e.g. identifiers). We would like to look at this, but we don't have much time for it right now. We'll most likely make an announcement (in gh discussions or just answer in threads like this one) when we'll be picking this topic.

cc: @sfc-gh-sthyagaraj

ddemara-indeed commented 4 months ago

Does V1 remove snowflake_role_grants? it seems that removing something that would save costs on users when theres no immediate plan to remedy with snowflake_grant_account_role might not be a great idea?

sfc-gh-asawicki commented 4 months ago

Hey @ddemara-indeed.

In fact, snowflake_role_grants has already been removed from the provider's 0.93.0 version. If this is an inconvenience, you can stay on the older version, at least for the time being.

We do not plan to address this issue before the V1. However, we understand the problem and recently received multiple inquiries about it. We will discuss this topic internally in the context of our V1 roadmap and consider our options.

In the meantime, we are open to contributions (check contribution guidelines).