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

snowflake_grant_privileges_to_account_role to support OWNERSHIP role #2549

Closed
AndrewKlimovski opened this issue Feb 23, 2024 · 10 comments · Fixed by #2605
Closed

snowflake_grant_privileges_to_account_role to support OWNERSHIP role #2549

AndrewKlimovski opened this issue Feb 23, 2024 · 10 comments · Fixed by #2605
Assignees
Labels
general-usage General help/usage questions

Comments

@AndrewKlimovski
Copy link
Contributor

AndrewKlimovski commented Feb 23, 2024

Terraform CLI and Provider Versions

v1.5.4

Use Cases or Problem Statement

Usage of snowflake_<resource>_grant is being deprecated in favour of snowflake_grant_privileges_to_account_role as per the deprecation warning.

Warning: Deprecated Resource
│ 
│   with snowflake_database_grant.grant_db_dev_data_lake,
│   on database.tf line 15, in resource "snowflake_database_grant" "grant_db_dev_data_lake":
│   15: resource "snowflake_database_grant" "grant_db_dev_data_lake" {
│ 
│ This resource is deprecated and will be removed in a future major version release. Please use snowflake_grant_privileges_to_account_role instead.

Example snowflake_database_grant accepts the privilege OWNERSHIP however trying to move to the proposed new function throws the error:

Error: Unsupported privilege 'OWNERSHIP'
|
|
|
Granting ownership is only allowed in dedicated resources (snowflake_user_ownership_grant, snowflake_role_ownership_grant)

Proposal

Either the function snowflake_grant_privileges_to_account_role supports the 'OWNERSHIP' role or examples are give of how to port from the deprecated snowflake_<resource>_grant function to the new function

How much impact is this issue causing?

Medium

Additional Information

No response

@AndrewKlimovski AndrewKlimovski added the feature-request Used to mark issues with provider's missing functionalities label Feb 23, 2024
@sfc-gh-asawicki sfc-gh-asawicki added general-usage General help/usage questions and removed feature-request Used to mark issues with provider's missing functionalities labels Feb 23, 2024
@lachniej
Copy link

lachniej commented Feb 23, 2024

The same thing is happening with the snowflake_grant_priviliges_to_database_role on attempted OWNERSHIP grants, could we add similar modification of that role as well?

@AndrewKlimovski
Copy link
Contributor Author

@sfc-gh-jcieslak to be clear there are a number of functions that being deprecated and all have the same behaviour, I've just listed one of them in the example and @lachniej has called out another. Would be amazing to support all deprecated functions

@sfc-gh-jcieslak
Copy link
Collaborator

sfc-gh-jcieslak commented Feb 26, 2024

Hey @AndrewKlimovski @lachniej 👋
We wanted to create another resource that will be specialized in granting ownership as already mentioned snowflake_grant_priviliges_to_database_role and snowflake_grant_priviliges_to_account_role are already complex resources. Additionally granting ownership comes with its features and many edge cases that would add to the complexity and maintainability of those resources. That said, we'll add a new resource that will be used only for ownership transfer operations. I should start the work on the implementation this week.

@AndrewKlimovski
Copy link
Contributor Author

Thanks for the update @sfc-gh-jcieslak

Not sure what you mean by "as already mentioned" can you please clarify?

Would love to avoid double handling my SQL translations if possible.

Thanks

@sfc-gh-jcieslak
Copy link
Collaborator

sfc-gh-jcieslak commented Feb 26, 2024

Sure, you mentioned snowflake_grant_priviliges_to_account_role and @lachniej snowflake_grant_priviliges_to_database_role. I just wanted to clarify that in those resources OWNERSHIP won't be an accepted privilege (like right now, we'll error out, but will point you with the message to the new grant ownership resource once it's available). Granting OWNERSHIP will be handled by the newly introduced resource.

@Bryan-Meier
Copy link

@sfc-gh-jcieslak Thanks for clarifying and confirming the approach. The talk about creating a separate resource for handling the specialized ownership grant has been around for about 6 months in various threads I have been on and read. Is there a rough ETA of when we could expect to see a resource like this? More curious than anything because we have some jenky workarounds in place right now for this.

@lachniej
Copy link

lachniej commented Mar 7, 2024

@Bryan-Meier it looks like they are actively working on it #2604

@Bryan-Meier
Copy link

Bryan-Meier commented Mar 7, 2024

YAY!!! I missed #2604. Thanks for pointing that out @lachniej. I can't tell you how much complexity and maintenance this is going to relieve for us!

sfc-gh-jcieslak added a commit that referenced this issue Mar 14, 2024
The first part of the implementation of the `snowflake_grant_ownership`
resource. This is a "basic" version of this resource providing baseline
functionalities needed to transfer ownership in Terraform. In the next
pull request, I'll add all of the edge cases we have to cover (most of
them are described
[here](https://docs.snowflake.com/en/sql-reference/sql/grant-ownership#usage-notes)).

Changes made:
- Created a new `snowflake_grant_ownership` resource with CRUD
operations implemented (still there are TODOs left for discussion)
- Added examples and documentation needed for the resource and its
identifier

Things to do before the merge:
- remove `snowflake_grant_ownership` from the provider.go

TODO in the next pr(s):
- Add deprecation messages to old grant resources specifically made for
granting ownership
- Add edge cases and test them (and if needed describe them in the
documentation and add examples)
- Add `setId("")` in read and forcefully grant ownership in Create
operation
- Referring to
[comment](#2604 (comment)),
test different cases where the Delete operation may struggle with
- Test outside of Terraform interactions to see how it behaves in
different situations

## Test Plan
* [x] acceptance tests
* [x] unit tests for the resource identifier conversions from/to String
representation
* [x] unit tests for the helper functions needed by resource CRUD
operations

## References
* [GRANT
OWNERSHIP](https://docs.snowflake.com/en/sql-reference/sql/grant-ownership)

## Mentioned in
A list of issues requesting this resource (a big probability there's
more); notify after part 2 will be done.
- #2549
- #2199
- #2084
- #1942
- #1875
sfc-gh-swinkler pushed a commit that referenced this issue Mar 19, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.87.3-pre](v0.87.2...v0.87.3-pre)
(2024-03-18)


### 🎉 **What's new:**

* Add snowflake grant ownership resource
([#2604](#2604))
([bfadd24](bfadd24)),
closes
[#2549](#2549)
[#2199](#2199)
[#2084](#2084)
[#1942](#1942)
[#1875](#1875)


### 🔧 **Misc**

* Fix env variables for tests
([#2603](#2603))
([8bc2437](8bc2437))
* release 0.87.3-pre
([a2be7b9](a2be7b9))


### 🐛 **Bug fixes:**

* alter table column data type
([#2607](#2607))
([538b6dc](538b6dc))
* cgo goreleaser alt solution
([#2613](#2613))
([5d31856](5d31856))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: snowflake-release-please[bot] <105954990+snowflake-release-please[bot]@users.noreply.github.com>
@sfc-gh-jcieslak
Copy link
Collaborator

Reopening, because it auto-closed :/

sfc-gh-jcieslak added a commit that referenced this issue Apr 3, 2024
A follow-up for #2604. 

Done in this pr:
- Add setId("") in Read (when ownership is not found on the target
object) and forcefully grant ownership in Create (this was already
present, but added test cases for it).
- Edge cases
- Granting `ON PIPE` and `ON ALL PIPES` is handled (pipes are paused
before and resumed after ownership transfer)

Full list of things that still need to be done:
- Deprecation messages
- More documentation (explain how grant_ownership resource handles edge
cases) and examples that would show simple usage, edge cases, cases
where the resource may cause trouble
- Referring to
#2604 (comment),
test different cases where the Delete operation may struggle with
- Test outside of Terraform interactions to see how it behaves in
different situations
- A test where used role is not privileged enough to transfer ownership
- Also cases within Terraform to see how grant_ownership will act with
other grant resources within certain configurations
- Edge cases
  - Granting `ON TASK`
  - Use `VIEW` when granting on `MATERIALIZED VIEW`
  - Granting `ON EXTERNAL TABLES`

## References
[GRANT
OWNERSHIP](https://docs.snowflake.com/en/sql-reference/sql/grant-ownership)

## Mentioned in
A list of issues requesting this resource: #2549 #2199 #2084 #1942 #1875
sfc-gh-jcieslak added a commit that referenced this issue Apr 8, 2024
A follow-up for
#2604.

Done in this pr:
- All of the edge cases handled and tested (except of tasks that are
done in the separate pr):
  - Materialized views (already handled by Snowflake no changes needed)
  - RBAC hierarchy (test case added)
- Delete dependent resource (role or granted object) and remove grant
resource from the state (test case added)

Won't do:
- External tables (cannot handle this edge case, because we have to know
the auto_refresh state of the external table; it's not retrievable by
SHOW or DESC commands. It will be still possible to grant ownership of
the external table, but there may be additional manual work to do
afterward. Everything is documented.)

## Test Plan
<!-- detail ways in which this PR has been tested or needs to be tested
-->
* [x] acceptance tests that show how the resource is handling certain
edge cases + RBAC use case

## References
[GRANT
OWNERSHIP](https://docs.snowflake.com/en/sql-reference/sql/grant-ownership)

## Mentioned in
A list of issues requesting this resource:
#2549
#2199
#2084
#1942
#1875
@sfc-gh-jcieslak
Copy link
Collaborator

Hey 👋
Closing, as the issue was about the deprecated/not allowed functionality. Recently, we released a new grant resource which is capable of granting ownership. Please, give it a try. If there will be any issues with it, create another GitHub issue. Also, please check our technical documentation section where you can find a migration guide that can help you with upgrading to the latest grant resources and our newly added design decision doc (regarding new grant resources).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general-usage General help/usage questions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants