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: support custom ownership transfer for grants #1743

Merged
merged 7 commits into from
May 30, 2023
Merged

feat: support custom ownership transfer for grants #1743

merged 7 commits into from
May 30, 2023

Conversation

hexDoor
Copy link
Contributor

@hexDoor hexDoor commented Apr 24, 2023

  • Add new optional schema variable revert_ownership_to_role_name which allows granular control over which role ownership will transfer to when destroying generic grants (including destruction during update) such as the following:
    • database_grant
    • external_table_grant
    • file_format_grant
    • function_grant
    • integration_grant
    • masking_policy_grant
    • materialized_view_grant
    • pipe_grant
    • procedure_grant
    • row_access_policy_grant
    • schema_grant
    • sequence_grant
    • stage_grant
    • stream_grant
    • table_grant
    • tag_grant
    • task_grant
    • view_grant
    • warehouse_grant

Note: Grants that do not seem to have a concept of ownership such as resource_monitor_grant do not gain this optional variable.

  • Add RevokeOwnership method to *GrantExecutable interfaces and implement

    • Create new tests and modify existing ones as necessary
  • Fix small variable naming issues

    • AllGrantBuilder referred as fgb => agb
    • ExistingGrantExecutable referred as fge => ege

Test Plan

  • acceptance tests (shouldn't be necessary considering these changes are theoretically backwards compatible)
  • unit tets

References

@hexDoor hexDoor marked this pull request as ready for review April 26, 2023 00:56
@sfc-gh-swinkler
Copy link
Collaborator

sfc-gh-swinkler commented Apr 29, 2023

I think it would make more sense to have an dedicated role_ownership_grant resource than to add an attribute which doesn't do anything most of the time. However, I do plan to deprecate these existing grant resources in the future (see: #1470 for more info), so i guess its okay.

@sfc-gh-swinkler
Copy link
Collaborator

@hexDoor there appears to be some merge conflicts which must be resolved through command line. Could you please fix this?

Also I would like to let you know about a redesign effort to make a more stable and testable SDK. Refer to: #1767.

@hexDoor
Copy link
Contributor Author

hexDoor commented Apr 30, 2023

@sfc-gh-swinkler Apologies for the delay.

Merge conflicts have been resolved.

I think it would make more sense to have an dedicated role_ownership_grant resource than to add an attribute which doesn't do anything most of the time. However, I do plan to deprecate these existing grant resources in the future (see: #1470 for more info), so i guess its okay.

I agree but I found that this PR would be the least painful and disruptive way of implementing something that will unblock our project. I'll be happy to contribute to the discussion when I can.

Also I would like to let you know about a redesign effort to make a more stable and testable SDK. Refer to: #1767.

I have noted this if I need to implement new features in the future but I suggest continuing with the v1 design for this particular PR as it's meant to be more of a bandaid than a permanent fix. The aforementioned re-design of grant resources should remedy this in the future.

@@ -13,7 +13,6 @@ import (

var validUserPrivileges = NewPrivilegeSet(
privilegeMonitor,
privilegeOwnership,
Copy link
Contributor Author

@hexDoor hexDoor May 10, 2023

Choose a reason for hiding this comment

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

I remove privilegeOwnership as it should already be serviced by UserOwnershipGrant resource.
Leaving it in may contribute to confusion.

@sfc-gh-swinkler
Copy link
Collaborator

I have noted this if I need to implement new features in the future but I suggest continuing with the v1 design for this particular PR as it's meant to be more of a bandaid than a permanent fix. The aforementioned re-design of grant resources should remedy this in the future.

True, lets revisit this in the future. For now, i agree a bandaid solution is sufficient.

@sfc-gh-swinkler sfc-gh-swinkler merged commit eaa6e01 into Snowflake-Labs:main May 30, 2023
@sfc-gh-swinkler
Copy link
Collaborator

@hexDoor thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants