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.

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

[Bug]: Snowflake_Role regression - errors when parentheses are included in the role name #3127

Closed
1 task
jasonjoneszywave opened this issue Oct 10, 2024 · 13 comments
Labels
bug Used to mark issues with provider's incorrect behavior

Comments

@jasonjoneszywave
Copy link

Terraform CLI Version

1.7.0

Terraform Provider Version

0.95.0

Terraform Configuration

Previously we've been able to create roles with names that include parentheses in Snowflake via the Terraform provider without issue. Functionality performs as expected through 0.93.0.

I tried upgrading to 0.95.0 and now get the below error:

Error: unable to parse identifier: "TEST ROLE - (dev)", currently identifiers containing opening and closing parentheses '()' are not supported in the provider

Same behavior is observed whether the snowflake_role or the snowflake_account_role resource is used. I was unable to find any documentation of this behavior change in the change log or the migration guide.

This issue appears to tie back to the below commit related to 0.94.0.

https://github.com/Snowflake-Labs/terraform-provider-snowflake/commit/32c76906c2eb8571e4a995df20f6450ddd6f94b4#diff-763615a19db3cf298ad395f66b38d48a392f0fab8408c5c88596a6cc9f2af898

Category

category:resource

Object type(s)

resource:role

Expected Behavior

Successfully create a role with a name that includes parentheses. Ex. "TEST_ROLE - (dev)"

Actual Behavior

Error: unable to parse identifier: "TEST ROLE - (dev)", currently identifiers containing opening and closing parentheses '()' are not supported in the provider

Steps to Reproduce

  1. Define either a snowflake_role or snowflake_account_role resource with a name that includes parentheses like the examples below.

resource "snowflake_role" "test_role" {
name = "TEST ROLE - (dev)"
}

resource "snowflake_account_role" "test_account_role" {
name = "TEST ACCOUNT ROLE - (dev)"
}

  1. Execute a plan and apply.
  2. Error like below will be returned.

Error: unable to parse identifier: "TEST ROLE - (dev)", currently identifiers containing opening and closing parentheses '()' are not supported in the provider

How much impact is this issue causing?

Medium

Logs

No response

Additional Information

I have not found a workaround for this issue as of yet. Our naming convention for SCIM groups includes parentheses to enclose the environment the group pertains to. This naming convention is widely used in our organization and this regression results in a breaking change for us with no way forward for upgrading to versions of the provider above 0.93.0.

Would you like to implement a fix?

  • Yeah, I'll take it 😎
@jasonjoneszywave jasonjoneszywave added the bug Used to mark issues with provider's incorrect behavior label Oct 10, 2024
@sfc-gh-asawicki
Copy link
Collaborator

Hey @jasonjoneszywave. Thanks for reaching out to us.

The change is mentioned here: https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/technical-documentation/identifiers_rework_design_decisions.md#known-limitations-and-identifier-recommendations (the doc was linked e.g. here: https://github.com/Snowflake-Labs/terraform-provider-snowflake/releases/tag/v0.95.0).

However, the document mentions this limitation only for identifiers containing parentheses (like functions) and not for other resource types (like the role mentioned). This may still be a problem, though, in places where we don't know what type of identifier we need to handle (that's why the general limitation makes sense in some cases).

I see that we have an open TODO here:

// TODO(SNOW-1571674): Remove the validation
.

I will consult with the team on this topic in the morning and will let you know if and when we can loosen these restrictions, at least for the role resource.

@jasonjoneszywave
Copy link
Author

Hey @sfc-gh-asawicki. Any updates on this after meeting with the team?

@sfc-gh-asawicki
Copy link
Collaborator

Hey @jasonjoneszywave.

After initial discussions, we agreed that we have to loosen these restrictions for objects with ids not ending with (...). We want to make sure that it won't break the logic in other places, though. We have it scheduled for early next week, so that it should make the cut in the next week's release. I will keep you posted if anything changes.

@jasonjoneszywave
Copy link
Author

Thank you for the update @sfc-gh-asawicki I appreciate it!

Yeah, there's a lot to test for sure. Making sure it works with grants amongst other things will be critical as well.

@sfc-gh-asawicki
Copy link
Collaborator

Hey @jasonjoneszywave. Just letting you know, that the next release will happen next week or even the week after. Sorry for the inconvenience.

@jasonjoneszywave
Copy link
Author

Hey @sfc-gh-asawicki . Any updates on when we can expect this release?

sfc-gh-asawicki added a commit that referenced this issue Nov 7, 2024
sfc-gh-asawicki added a commit that referenced this issue Nov 7, 2024
sfc-gh-asawicki added a commit that referenced this issue Nov 7, 2024
sfc-gh-asawicki added a commit that referenced this issue Nov 8, 2024
Apply various fixes:
- Fix handling compute pool privileges (#2717)
- Fail to reproduce the problem with password policy user attachment
(#3005)
- Adapt user to BCR Bundle 2024_08 (#3125)
- Loosen identifier validations - parentheses (#3127) - check below
- Prove MANAGE SHARE TARGET works correctly (#3153)

On the identifier validation topic:
ParseIdentifierString should generally allow parentheses. It should
validate them for the identifiers for functions, procedures, etc.
Because of that:
- this validation was removed
- method usages were analyzed to check what consequences it has
throughout the provider
  - DecodeSnowflakeAccountIdentifier - OK, account level identifier
  - DecodeSnowflakeParameterID
- buildOptsForGrantsOn (grants datasource) - NOK, had to fix the logic
- ContainsIdentifierIgnoringQuotes - OK, transitively used only in
network policies
    - TestDecodeSnowflakeParameterID - OK
    - IsValidIdentifier - OK, used for other identifier types
- pkg/resource - OK, used in streams, table constraints and tag masking
policy associations
  - suppressIdentifierQuoting
- used in non-grant resources with non-argument identifier types - OK
- used in grant resources - OK, the validation will be relaxed for now,
diff suppression won't work correctly for the identifiers with
arguments, will be addressed with functions/procedures rework
(multi-field validation could be handled for such cases, issue added;
references:
hashicorp/terraform-plugin-sdk#354,
hashicorp/terraform-plugin-sdk#233)
- suppressIdentifierQuotingPartiallyQualifiedName - as above; currently
used only for streams
- parseIdentifier - used by other identifier types (type constraints
added)
- ParseObjectIdentifierString - OK, used for other identifier types
(ParseSchemaObjectIdentifierWithArguments is dedicated for identifier
with arguments)
- ParseSchemaObjectIdentifierWithArguments - OK, we split the input
string on first opening paren (so there are no other opening parens
there)
- Test_ParseIdentifierString - tests adjusted for the removed validation

Others:
- Remove unused privileges.go file
- Fix preview resources list for V1

References:
-
#2717
-
#3005
-
#3125
-
#3127
-
#3153
@sfc-gh-asawicki
Copy link
Collaborator

Hey @jasonjoneszywave. Today :)

sfc-gh-jmichalak pushed a commit that referenced this issue Nov 8, 2024
##
[0.98.0](v0.97.0...v0.98.0)
(2024-11-08)

Feature scope readiness for V1:
[link](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/v1-preparations/ESSENTIAL_GA_OBJECTS.MD)
([Roadmap
reference](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#wrap-up-the-functional-scope)).
:exclamation: Migration guide: [v0.97.0 ->
v0.98.0](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md#v0970--v0980)

### 🎉 What's new
- New resources:
- authentication_policy
([#3098](#3098)),
references
[#2880](#2880)
- external_volume
([#3106](#3106)),
partially references
[#2980](#2980)
- stream_on_directory_table
([#3129](#3129))
- stream_on_view
([#3150](#3150))
- primary_connection, secondary_connection
([#3162](#3162))
- secret_with_basic_authentication, secret_with_generic_string,
secret_with_oauth_authorization_code_grant,
secret_with_oauth_client_credentials
([#3110](#3110)),
([#3141](#3141))
- New data sources:
- connections
([#3155](#3155)),
([#3173](#3173))
- secrets
([#3131](#3131))
- Reworked:
- provider configuration hierarchy
([#3166](#3166)),
references
[#1881](#1881),
[#2145](#2145),
[#2925](#2925),
[#2983](#2983),
[#3104](#3104)
- provider configuration fields
([#3152](#3152))
streams data source
([#3151](#3151))
- SDK upgrades:
- Upgrade tag SDK
([#3126](#3126))
- Recreate streams when they are stale
([#3129](#3129))
### 🔧  Misc
- Add object renaming research summary
([#3172](#3172))
- Test support for object renaming
([#3130](#3130)),
([#3147](#3147)),
([#3154](#3154))
- Add tests to issue
[#3117](#3117)
([#3133](#3133))
- New roadmap entry
([#3158](#3158))
- Test more authentication methods
([#3178](#3178))
- Minor fixes
([#3174](#3174))
### 🐛  Bug fixes
- Apply various fixes
([#3176](#3176)),
this addresses BCR 2024_08, references
[#2717](#2717),
[#3005](#3005),
[#3125](#3125),
[#3127](#3127),
[#3153](#3153)
- Connection and secret data sources tests
([#3177](#3177))
- Fix grant import docs
([#3183](#3183)),
resolves
[#3179](https://github.com/Snowflake-Labs/terraform-provider-snowflake/discussions/3179)
- Fix user resource import
([#3181](#3181))
- Handle external type changes in stream resources
([#3164](#3164))
- Do not use OR REPLACE on initial creation in resources with
copy_grants
([#3129](#3129))
- Address issue
[#2201](#2201)
by introducing new stream resources

Co-authored-by: snowflake-release-please[bot] <105954990+snowflake-release-please[bot]@users.noreply.github.com>
@jasonjoneszywave
Copy link
Author

Looks like it hasn't made it to the Terraform registry yet, but once it does I'll try it out and report back.

@sfc-gh-asawicki
Copy link
Collaborator

Hey @jasonjoneszywave. Unfortunately, we had some technical problems on late Friday evening and the release was unsuccessful. Today, the whole team is absent (because of the national holiday in Poland), and we will prioritize 0.98.0 release tomorrow morning.

@sfc-gh-jmichalak
Copy link
Collaborator

Hi @jasonjoneszywave 👋

We've released a new v0.98.0 version (release, migration guide) with a fix. We're sorry for the delay.

@jasonjoneszywave
Copy link
Author

Thanks @sfc-gh-asawicki @sfc-gh-jmichalak !

I'm planning to do some testing/validation tomorrow. Will report back on how it goes.

@jasonjoneszywave
Copy link
Author

jasonjoneszywave commented Nov 13, 2024

@sfc-gh-asawicki @sfc-gh-jmichalak I did some testing this morning and was able to successfully Terraform granting an account role with () in the name to another account role and also was able to successfully grant an account role with () in the name usage on a warehouse and a database.

Not exhaustive testing by any means, but looking good so far. Will let you know if we hit any issues. Thanks again. This fix helps us a ton!

@sfc-gh-asawicki
Copy link
Collaborator

I'm glad to hear that. I will close the issue then. Please let us know if you encounter any more problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to mark issues with provider's incorrect behavior
Projects
None yet
Development

No branches or pull requests

3 participants