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: tag based masking policy #1143

Merged

Conversation

berosen
Copy link
Contributor

@berosen berosen commented Jul 18, 2022

  • Adds the ability to attach a masking policy to a tag

closes #1116

Note SNOWFLAKE_WAREHOUSE needs to be provided for this resource to query the POLICY_REFERENCES table function.

Test Plan

  • acceptance tests
  • unit tests

Tested with TF_ACC=1 go test -v ./... -run TestAcc_TagMaskingPolicyAssociation

References

@sfc-gh-swinkler
Copy link
Collaborator

sorry for the long delay in responding to this PR, my wife died in July and I was on grievance leave. I have been catching up over the past couple weeks. This is a complicated PR so i have been thinking about it for a few days now.

We are actually in the process of changing how tags work in the provider. It won't break backwards compatibility, but it does affect how new resources that involved tagging should be implemented. What I would like to do is wait until we get the change implemented, which should be later this week, and then address this masking policy attachment after that. I do like this PR, but there will need to be a few changes to get it to align with the new model. If you are not able to make the change, then i can take over and make sure this gets done in the next sprint.

@berosen
Copy link
Contributor Author

berosen commented Aug 17, 2022

sorry for the long delay in responding to this PR, my wife died in July and I was on grievance leave. I have been catching up over the past couple weeks. This is a complicated PR so i have been thinking about it for a few days now.

We are actually in the process of changing how tags work in the provider. It won't break backwards compatibility, but it does affect how new resources that involved tagging should be implemented. What I would like to do is wait until we get the change implemented, which should be later this week, and then address this masking policy attachment after that. I do like this PR, but there will need to be a few changes to get it to align with the new model. If you are not able to make the change, then i can take over and make sure this gets done in the next sprint.

I'm really sorry to hear that, sending you my condolences.

Sounds good to me. Feel free to keep me posted with what changes need to be made once the implementation change is complete. I'll let you know if I'm unable to work on them in a timely manner. Thank you for taking a look at this.

@sfc-gh-swinkler
Copy link
Collaborator

basically how it will work is that there is a new resource called "snowflake_tag_attachment". it will replace the current tag schema on each resource. below is sample configuration

resource "snowflake_tag_attachment" "cost_center_tag_attachment" {
tag_id = snowflake_tag.cost_center.id
value = "finance"
resource_id = snowflake_database.my_db.id
resource_type = "DATABASE"
}

as you can see we are using a tag_id rather than database, schema and name. the tag_id is a string which has the same infomration, and is flexible in the way that the data is passed in, for example:
"database"."schema"."name"
"database.schema.name"
"database|schema.name" (structure of the snowflake_tag resource id)

are all allowed.

We should be getting this PR done either tomorrow or Monday.

@sfc-gh-swinkler
Copy link
Collaborator

looking good, looking real good. as long as this passes integration tests i will merge this

@sfc-gh-swinkler
Copy link
Collaborator

/ok-to-test sha=32e493e

@github-actions
Copy link

Integration tests failure for 32e493e

@berosen
Copy link
Contributor Author

berosen commented Aug 26, 2022

@sfc-gh-swinkler I've updated the CI here to include SNOWFLAKE_WAREHOUSE. Could you add a value for this in your CI secrets? That should fix the failing test.

@sfc-gh-swinkler
Copy link
Collaborator

@berosen that environment variable is already set

@sfc-gh-swinkler
Copy link
Collaborator

/ok-to-test sha=227a37f

@github-actions
Copy link

Integration tests failure for 227a37f

1 similar comment
@github-actions
Copy link

Integration tests failure for 227a37f

@sfc-gh-swinkler
Copy link
Collaborator

/ok-to-test sha=227a37f

@github-actions
Copy link

Integration tests failure for 227a37f

@sfc-gh-swinkler
Copy link
Collaborator

not sure what is going on here. the SNOWFLAKE_WAREHOUSE environment variable is definitely set on the repo settings. I also updated it just in case
Screen Shot 2022-08-26 at 2 32 41 PM

Its also not clear to me why this even needs to be set for the masking policy association. Nowhere in the documentation does it state that this needs to be set: https://docs.snowflake.com/en/user-guide/tag-based-masking-policies.html. As far as i know this only needs to be set for Python UDF. Are you setting this locally when you test the resource? If so, what exact line is causing this to fail if the environment variable is not set? Maybe can add some debugging there to figure out what the problem is. Because default warehouse gets set in the provider from environment variable here:
https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/pkg/provider/provider.go#L366

@github-actions
Copy link

Integration tests failure for 227a37f

@berosen
Copy link
Contributor Author

berosen commented Aug 26, 2022

hmm that's odd. Yes I'm setting that variable locally. The line that needs the warehouse is this one because it uses the policy_references table function described in the docs here in step 5. I'm not super familiar with GitHub actions but I'm wondering if this is what's going on and the CI change just isn't being picked up.

@sfc-gh-swinkler sfc-gh-swinkler merged commit e388545 into Snowflake-Labs:main Aug 31, 2022
@sfc-gh-swinkler
Copy link
Collaborator

going to merge this and try to figure out what the problem is on my own. i think you are right, that its probably not picking up the changes.

@sfc-gh-swinkler
Copy link
Collaborator

thank you for your contribution

@berosen
Copy link
Contributor Author

berosen commented Aug 31, 2022

Thanks @sfc-gh-swinkler , look like that was the issue and ended up passing here. Appreciate your help on this.

@berosen berosen deleted the feature/tag-masking-policy branch August 31, 2022 19:56
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.

Add ability to set a masking policy on a tag
3 participants