[feature] Network Policy Resource #271

grahamplace · 2020-10-09T17:05:17Z

Adds support for managing Snowflake Network Policy objects via the new snowflake_network_policy resource type.

Test Plan

acceptance tests

tested built provider locally with trial Snowflake account

References

ryanking

Thanks! This looks great.

A few minor bits of feedback and this should be good to go.

Thanks for writing acceptance tests.

pkg/provider/provider.go

pkg/resources/network_policy.go

ryanking · 2020-10-09T17:23:33Z

pkg/resources/network_policy_acceptance_test.go

+	name            = "%v"
+	comment         = "%v"
+	allowed_ip_list = ["192.168.0.100/24", "29.254.123.20"]
+	blocked_ip_list = ["192.168.0.101"]


We should probably include testing adding to users here.

I've added a users list to the test network policy resource definition (and a dummy user resource definition) so at least the user-granting code should be executing during this acceptance test now.

However, (as far as I can tell) I have to define:
ImportStateVerifyIgnore: []string{"set_for_account", "users"},

because nowhere in ReadNetworkPolicy is there:
data.Set("set_for_account", ...) or
data.Set("users", ...)

This is the case because there doesn't appear to be a way in Snowflake to do "get all users with this Network Policy set" or "tell me if this network policy is set for the Snowflake account". Neither SHOW nor DESC returns any information about the active/applied status of a given Network Policy:

Show: https://docs.snowflake.com/en/sql-reference/sql/show-network-policies.html

Describe: https://docs.snowflake.com/en/sql-reference/sql/desc-network-policy.html

Is it acceptable to leave those out of ReadNetworkPolicy (and thus having to include them in ImportStateVerifyIgnore)?

FWIW this is why I had to write this resource with ForceNew: true for those two fields, with the "exclusive assignment" explanation in the docs — the simplest way I could figure out how to deal with this is to just always destroy/recreate and re-grant to users or re-set on account

Ah that makes sense!

Thinking about this a bit more, I think we should consider separating the attachment to a separate resource. This should allow us to avoid the scenario where we need to recreate the network policy resource every time one of the attachments changes. Of course the attachments may need to be recreated, but that might be tolerable. What do you think?

I'm on board with this — a separate attachment resource was my original plan, but I moved away from it because I (erroneously) observed that if you dropped a network policy, the users with that policy set continued to be restricted by the policy. Hence, I thought we needed to keep the attachment tightly coupled with the resource.

However, I tested this understanding again and it turns out there's just a bit of lag (not more than a minute) between dropping the network policy and the user then being able to open a session from any IP address

So this approach should work. I'll update PR

I guess one thing to think about is that if we do this, the snowflake_network_policy_attachment resource wouldn't behave like the similar *_grant resources (e.g. warehouse/database/etc) and create exclusive attachments

Because there's not a way of getting existing attachments from SHOW/DESCing network policies/users/accounts, a network policy could be attached to a user outside of Terraform, and then changes to the snowflake_network_policy_attachment wouldn't destroy that attachment. That's probably fine? We should just document it

Yeah I think that is fine given the limitations we are working with.

Done — the one thing I'm unsure about is ReadNetworkPolicyAttachment — given above limitations, there's no way to actually implement schema.ReadFunc for the attachment resource. I don't think I actually need ReadNetworkPolicyAttachment implemented for this, but InternalValidate fails without it. To get around that, I just implemented a dummy func 🤷

lmk if there's a better way to handle this!

pkg/resources/network_policy_test.go

pkg/resources/network_policy.go

ryanking · 2020-10-09T17:35:49Z

Note that I am seeing some acceptance test failures, running make test-acceptance locally -

=== RUN   TestAccNetworkPolicy
    testing.go:683: Step 0 error: errors during apply:
        
        Error: 000606 (57P03): No active warehouse selected in the current session.  Select an active warehouse with the 'use warehouse' command.
        
        
          on /var/folders/ly/qwpw0g113mz15651mkc9wd400000gr/T/tf-test836548438/main.tf line 2:
          (source code not available)
        
        
    testing.go:744: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: errors during refresh: 000606 (57P03): No active warehouse selected in the current session.  Select an active warehouse with the 'use warehouse' command.
        
        
        State: <nil>

It seems that the select query needs a warehouse, which means we would need to add that to the provider configuration. In the past we've mostly avoided that and instead dealt with the the annoyances of working with just show and describe, which don't need a warehouse.

grahamplace · 2020-10-12T20:38:51Z

Note that I am seeing some acceptance test failures, running make test-acceptance locally -
=== RUN   TestAccNetworkPolicy
    testing.go:683: Step 0 error: errors during apply:
        
        Error: 000606 (57P03): No active warehouse selected in the current session.  Select an active warehouse with the 'use warehouse' command.
        
        
          on /var/folders/ly/qwpw0g113mz15651mkc9wd400000gr/T/tf-test836548438/main.tf line 2:
          (source code not available)
        
        
    testing.go:744: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: errors during refresh: 000606 (57P03): No active warehouse selected in the current session.  Select an active warehouse with the 'use warehouse' command.
        
        
        State: <nil>
It seems that the select query needs a warehouse, which means we would need to add that to the provider configuration. In the past we've mostly avoided that and instead dealt with the the annoyances of working with just show and describe, which don't need a warehouse.

Similar to my reply on your other comment — the issue here is that Snowflake's implementation of Network Policies is weird/somewhat different than other object types.

More specifically in this case, I think we have to do the RESULT_SCAN approach I used (which is the cause of acceptance tests user requiring a warehouse). This is because there is no way to SHOW/DESC the information about a single Network Policy in Snowflake SQL:

DESC is limited to a single Network Policy, but it only returns the IP address lists:
SHOW only allows SHOW NETWORK POLICIES — for some reason, unlike other object types, the output cannot be limited to a single Network Policy object via LIKE. Adding a LIKE does not limit the returned data:

Thus, in order to get the current comment on the Network Policy in question, the SHOW output has to be cut down to just name = "mypolicy" via a second query that uses RESULT_SCAN('original_query_id')

I worked around this by setting DEFAULT_WAREHOUSE on the Travis user. Not ideal obviously, but I don't have other ideas for how to implement ReadNetworkPolicy without the RESULT_SCAN approach given limitations outlined above. Open to ideas!

grahamplace · 2020-10-12T20:40:16Z

^ while writing that I realized that I can probably just refactor to do the SHOW/DESC post-processing in Go instead of in Snowflake SQL, thus bypassing the RESULT_SCAN and the need for a Warehouse

Still posted as it's useful context, but I'll take a stab at that refactor now

grahamplace · 2020-10-13T17:21:20Z

Okay, updated and ready for another look @ryanking!

aidan-melen · 2020-10-17T00:06:46Z

thanks for working on this feature @grahamplace and @ryanking!

alldoami

Ran test-acceptancec-ci locally and passed all the test! Thanks for the work! 🥳 I'll let @ryanking take one more look, but LGTM!

grahamplace · 2020-10-19T18:46:07Z

Thanks for the review @alldoami! Per Ryan's suggestion, I still need to refactor this to move the attachment into a separate resource. That work is on my todo list for today, so hopefully I can put that change up and tag you both for another look

grahamplace · 2020-10-20T00:13:11Z

Done! Ready for another look @ryanking @alldoami 🙂 Would appreciate input on this bit:
#271 (comment)

ryanking

I am currently getting acceptance test failures too:

pkg/resources/network_policy_attachment_acceptance_test.go:12:6: TestAccNetworkPolicyAttachment redeclared in this block
	previous declaration at pkg/resources/network_policy_acceptance_test.go:12:40
pkg/resources/network_policy_attachment_acceptance_test.go:40:6: networkPolicyAttachmentConfig redeclared in this block
	previous declaration at pkg/resources/network_policy_acceptance_test.go:40:55

ryanking · 2020-10-21T17:41:41Z

pkg/resources/network_policy_acceptance_test.go

+			{
+				ResourceName:      "snowflake_network_policy_attachment.test",
+				ImportState:       true,
+				ImportStateVerify: false,


Is there a reason for this to be false?

I set this to false while developing because I wasn't able to get it to work, I thought due to the inability to actually implement ReadNetworkPolicyAttachment

But it looks like it works now so switching it back to true

annnd I was wrong -- it's actually still not working:

=== RUN TestAccNetworkPolicyAttachment TestAccNetworkPolicyAttachment: testing.go:683: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected. (map[string]string) { } (map[string]string) (len=5) { (string) (len=19) "network_policy_name": (string) (len=10) "uowauijhoy", (string) (len=15) "set_for_account": (string) (len=5) "false", (string) (len=7) "users.#": (string) (len=1) "2", (string) (len=16) "users.2494182523": (string) (len=10) "test-user1", (string) (len=16) "users.3213322168": (string) (len=10) "test-user2" } --- FAIL: TestAccNetworkPolicyAttachment (7.03s)

I think because ReadNetworkPolicyAttachment isn't really implemented?

Just confirming you saw this last comment/commit @ryanking? Does the merge mean you're fine skipping ImportStateVerify given Snowflake limitations that prevent us from show/desc'ing anything about a policy attachment on users/account?

Yeah its fine.

Awesome, thanks for all the help getting this merged!

grahamplace · 2020-10-23T16:55:14Z

I am currently getting acceptance test failures too:

pkg/resources/network_policy_attachment_acceptance_test.go:12:6: TestAccNetworkPolicyAttachment redeclared in this block
	previous declaration at pkg/resources/network_policy_acceptance_test.go:12:40
pkg/resources/network_policy_attachment_acceptance_test.go:40:6: networkPolicyAttachmentConfig redeclared in this block
	previous declaration at pkg/resources/network_policy_acceptance_test.go:40:55

Ah oops, I accidentally overwrote network_policy_acceptance_test.go with network_policy_attachment_acceptance_test.go, should be corrected now

Adds support for managing Snowflake Network Policy objects via the new `snowflake_network_policy` resource type.  * [x] acceptance tests  * [x] tested built provider locally with trial Snowflake account  * Issue: Snowflake-Labs#221 * https://docs.snowflake.com/en/sql-reference/sql/create-network-policy.html

Adds support for managing Snowflake Network Policy objects via the new `snowflake_network_policy` resource type. ## Test Plan  * [x] acceptance tests  * [x] tested built provider locally with trial Snowflake account ## References  * Issue: Snowflake-Labs#221 * https://docs.snowflake.com/en/sql-reference/sql/create-network-policy.html

Adds support for managing Snowflake Network Policy objects via the new `snowflake_network_policy` resource type.  * [x] acceptance tests  * [x] tested built provider locally with trial Snowflake account  * Issue: Snowflake-Labs#221 * https://docs.snowflake.com/en/sql-reference/sql/create-network-policy.html

[feature] Network Policy Resource

f0718af

grahamplace requested a review from a team as a code owner October 9, 2020 17:05

grahamplace requested review from ryanking and removed request for a team October 9, 2020 17:05

grahamplace added 2 commits October 9, 2020 10:10

Bring branch up to date with latest main

81dd1af

Formatting

741b114

ryanking suggested changes Oct 9, 2020

View reviewed changes

grahamplace added 2 commits October 12, 2020 08:45

Move CREATE DDL into snowflake package

b586fe5

Add users to TestAccNetworkPolicy resource

e49327a

grahamplace added 5 commits October 13, 2020 08:44

Refactor to post-process rows in go

dfb1d8e

Test UPDATE in network policy acceptance test

6273dd5

Merge branch 'main' into graham/221-feature-network-policy

59c408a

Test set_for_account attr in network policy acceptance test

857883b

Test and fix unset blocked list case

81a919e

grahamplace added 2 commits October 13, 2020 11:31

Merge branch 'main' into graham/221-feature-network-policy

52006a8

Merge branch 'main' into graham/221-feature-network-policy

7b377e4

alldoami approved these changes Oct 19, 2020

View reviewed changes

grahamplace added 2 commits October 19, 2020 16:58

Split network policy attachment out into separate resource

4001248

Update docs

66b178d

grahamplace requested review from alldoami and ryanking October 20, 2020 15:51

ryanking suggested changes Oct 21, 2020

View reviewed changes

ryanking assigned grahamplace Oct 23, 2020

Restore correct network_policy_acceptance_test.go

1f9077b

ImportStateVerify: true in TestAccNetworkPolicyAttachment

4cfcb50

grahamplace requested a review from ryanking October 23, 2020 17:11

Turn off failing ImportStateVerify

53a3f9a

ryanking approved these changes Oct 23, 2020

View reviewed changes

ryanking merged commit 4dd6077 into Snowflake-Labs:main Oct 23, 2020

ryanking mentioned this pull request Dec 4, 2020

Feature request: Add support for network policy #94

Closed

ryanking mentioned this pull request Dec 16, 2020

[feature] network policy #221

Closed

adaco mentioned this pull request Mar 9, 2022

Destruction of non-account-level network policy attachments unsets any account-level network policy #900

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] Network Policy Resource #271

[feature] Network Policy Resource #271

grahamplace commented Oct 9, 2020

ryanking left a comment

ryanking Oct 9, 2020

grahamplace Oct 12, 2020

grahamplace Oct 12, 2020

ryanking Oct 13, 2020

grahamplace Oct 15, 2020

grahamplace Oct 15, 2020

ryanking Oct 15, 2020

grahamplace Oct 20, 2020

ryanking commented Oct 9, 2020

grahamplace commented Oct 12, 2020

grahamplace commented Oct 12, 2020

grahamplace commented Oct 13, 2020

aidan-melen commented Oct 17, 2020

alldoami left a comment •

edited

Loading

grahamplace commented Oct 19, 2020

grahamplace commented Oct 20, 2020

ryanking left a comment

ryanking Oct 21, 2020

grahamplace Oct 23, 2020

grahamplace Oct 23, 2020

grahamplace Oct 23, 2020

ryanking Oct 23, 2020

grahamplace Oct 23, 2020

grahamplace commented Oct 23, 2020

[feature] Network Policy Resource #271

[feature] Network Policy Resource #271

Conversation

grahamplace commented Oct 9, 2020

Test Plan

References

ryanking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanking commented Oct 9, 2020

grahamplace commented Oct 12, 2020

grahamplace commented Oct 12, 2020

grahamplace commented Oct 13, 2020

aidan-melen commented Oct 17, 2020

alldoami left a comment • edited Loading

Choose a reason for hiding this comment

grahamplace commented Oct 19, 2020

grahamplace commented Oct 20, 2020

ryanking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamplace commented Oct 23, 2020

alldoami left a comment •

edited

Loading