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

feat: Add snowflake grant ownership resource #2604

Merged
merged 15 commits into from
Mar 14, 2024

Conversation

sfc-gh-jcieslak
Copy link
Collaborator

@sfc-gh-jcieslak sfc-gh-jcieslak commented Mar 7, 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).

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, 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

  • acceptance tests
  • unit tests for the resource identifier conversions from/to String representation
  • unit tests for the helper functions needed by resource CRUD operations

References

Mentioned in

A list of issues requesting this resource (a big probability there's more); notify after part 2 will be done.

Copy link

github-actions bot commented Mar 7, 2024

Integration tests failure for 9c01b9c85e0c86918616ebfc4f4d360c53ecdecb

Copy link

github-actions bot commented Mar 7, 2024

Integration tests failure for cc173c2779042c7ff83d1517c659fa77f409122e

@sfc-gh-jcieslak sfc-gh-jcieslak force-pushed the add-snowflake-grant-ownership-resource branch from cc173c2 to 924b386 Compare March 7, 2024 14:51
Copy link

github-actions bot commented Mar 7, 2024

Integration tests failure for 924b38600c2a088e3714915ad06259297fb4e625

@sfc-gh-jcieslak sfc-gh-jcieslak marked this pull request as ready for review March 7, 2024 16:26
Copy link

github-actions bot commented Mar 7, 2024

Integration tests failure for d22e3d957e53e6ddb8deb84c717e8a80faf4832d

docs/resources/grant_ownership.md Outdated Show resolved Hide resolved
docs/resources/grant_privileges_to_database_role.md Outdated Show resolved Hide resolved
templates/resources/grant_ownership.md.tmpl Outdated Show resolved Hide resolved
pkg/sdk/testint/helpers_test.go Outdated Show resolved Hide resolved
pkg/sdk/object_types.go Show resolved Hide resolved
pkg/resources/grant_ownership.go Show resolved Hide resolved
pkg/resources/grant_ownership.go Show resolved Hide resolved
pkg/resources/grant_ownership.go Outdated Show resolved Hide resolved
pkg/resources/grant_ownership_test.go Outdated Show resolved Hide resolved
Copy link

Integration tests failure for 306e09f29127cf8bc3afbaa607b393843e9fce5f

Copy link

Integration tests success for 66ca523a80f6c939bfc42c4ccec09a3429f1f9ba

@sfc-gh-asawicki sfc-gh-asawicki self-requested a review March 13, 2024 13:10
"database_role_name": "account_role_name",
})

defer func() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be done with assert.Panics as suggested, you can change in the follow-up PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

Copy link

Integration tests failure for e855a9291ff0e7f79aa609d9640a7d7e2e304273

Copy link

Integration tests failure for 9737b07e62eec0b4503f80e99a690a80c92e9089

@sfc-gh-jcieslak sfc-gh-jcieslak merged commit bfadd24 into main Mar 14, 2024
8 of 9 checks passed
@sfc-gh-jcieslak sfc-gh-jcieslak deleted the add-snowflake-grant-ownership-resource branch March 14, 2024 09:31
sfc-gh-swinkler pushed a commit that referenced this pull request 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 added a commit that referenced this pull request 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 pull request 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 added a commit that referenced this pull request Apr 17, 2024
A follow-up for
#2604 (comment).
Updated target objects for privilege-granting resources based on
documentation (also added references above available object's list).

Pr also contains:
- #2689 is fixed in the pr and acceptance tests are added. 
- #2696 documentation fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants