Repository: chanzuckerberg/terraform-provider-snowflake (go)

Issue:
Destruction of non-account-level network policy attachments unsets any account-level network policy
**Provider Version**

`0.25.36`, but AFAICT the issue dates back to the resource's introduction in #271.

**Terraform Version**

`1.1.3` 

**Describe the bug**

Destruction of a `snowflake_network_policy_attachment` resource [unconditionally unsets any account-level network policy](https://github.com/chanzuckerberg/terraform-provider-snowflake/blob/main/pkg/resources/network_policy_attachment.go#L139), even when the `snowflake_network_policy_attachment`'s `set_for_account` attribute is set to `false`. This has the effect of unsetting account-level network policies that may have been configured outside of Terraform. `snowflake_network_policy` renames also trigger `snowflake_network_policy_attachment` re-creates, which exacerbates the issue.

**Expected behavior**

Destruction of a `snowflake_network_policy_attachment` resource with `set_for_account = false` shouldn't unset account-level network policies (because the resource itself should know that it _wasn't_ set at the account-level).

**Code samples and commands**

Define some network policy and an attachment:
```tf
resource snowflake_network_policy policy {
  name    = "policy"
  comment = "A policy."
  allowed_ip_list = ["192.168.0.100/24"]
  blocked_ip_list = ["192.168.0.101"]
}

resource snowflake_network_policy_attachment attach {
  network_policy_name = snowflake_network_policy.policy.name
  set_for_account     = false  // note it's not account-level here
  users = ["user1", "user2"]
}
```
Rename `snowflake_network_policy.policy` to trigger `snowflake_network_policy_attachment.attach` destroy + re-create:
```tf
resource snowflake_network_policy policy {
  name    = "some_new_name"  // changed here.
  comment = "A policy."
  allowed_ip_list = ["192.168.0.100/24"]
  blocked_ip_list = ["192.168.0.101"]
}

resource snowflake_network_policy_attachment attach {
  network_policy_name = snowflake_network_policy.policy.name
  set_for_account     = false
  users = ["user1", "user2"]
}
```
Check `snowflake.account_usage.query_history` and see that the user identity ran an `ALTER ACCOUNT UNSET NETWORK_POLICY` statement. 


Pull request description:
fix: Network Attachment (Set For Account)
Fixes #900 and also added Read Implementation

## Test Plan
<!-- detail ways in which this PR has been tested or needs to be tested -->
* [x] acceptance tests
<!-- add more below if you think they are relevant -->
* [x] Tests

## References
*  https://docs.snowflake.com/en/user-guide/network-policies.html#activating-a-network-policy-for-your-account
*  https://docs.snowflake.com/en/user-guide/network-policies.html#identifying-a-network-policy-activated-at-the-account-or-user-level

Patch:
diff --git a/pkg/resources/network_policy_attachment.go b/pkg/resources/network_policy_attachment.go
index cc4dd2d1..e7e1ebbb 100644
--- a/pkg/resources/network_policy_attachment.go
+++ b/pkg/resources/network_policy_attachment.go
@@ -2,6 +2,8 @@ package resources
 
 import (
 	"database/sql"
+	"log"
+	"strings"
 
 	"github.com/chanzuckerberg/terraform-provider-snowflake/pkg/snowflake"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
@@ -70,13 +72,60 @@ func CreateNetworkPolicyAttachment(d *schema.ResourceData, meta interface{}) err
 		}
 	}
 
-	return nil
+	return ReadNetworkPolicyAttachment(d, meta)
 }
 
 // ReadNetworkPolicyAttachment implements schema.ReadFunc
 func ReadNetworkPolicyAttachment(d *schema.ResourceData, meta interface{}) error {
-	// HACK: InternalValidate requires Read to be implemented
-	// There is no way of using SHOW/DESC on Network Policies/Users to pull attachment information, so we can't actually Read
+	db := meta.(*sql.DB)
+	policyName := strings.Replace(d.Id(), "_attachment", "", 1)
+	builder := snowflake.NetworkPolicy(policyName)
+
+	var currentUsers []string
+
+	err := d.Set("network_policy_name", policyName)
+	if err != nil {
+		return err
+	}
+
+	if u, ok := d.GetOk("users"); ok {
+		users := expandStringList(u.(*schema.Set).List())
+		for _, user := range users {
+			row := snowflake.QueryRow(db, builder.ShowOnUser(user))
+			attachment, err := snowflake.ScanNetworkPolicyAttachment(row)
+			if err == sql.ErrNoRows {
+				log.Printf("[DEBUG] network policy (%s) not found on user (%s)", d.Id(), user)
+				continue
+			}
+
+			if attachment.Level.String == "USER" && attachment.Key.String == "NETWORK_POLICY" && attachment.Value.String == policyName {
+				currentUsers = append(currentUsers, user)
+			}
+		}
+
+		err := d.Set("users", currentUsers)
+		if err != nil {
+			return err
+		}
+	}
+
+	isSetOnAccount := false
+	row := snowflake.QueryRow(db, builder.ShowOnAccount())
+	attachment, err := snowflake.ScanNetworkPolicyAttachment(row)
+	if err == sql.ErrNoRows {
+		log.Printf("[DEBUG] network policy (%s) not found on account", d.Id())
+		isSetOnAccount = false
+	}
+
+	if err == nil && attachment.Level.String == "ACCOUNT" && attachment.Key.String == "NETWORK_POLICY" && attachment.Value.String == policyName {
+		isSetOnAccount = true
+	}
+
+	err = d.Set("set_for_account", isSetOnAccount)
+	if err != nil {
+		return err
+	}
+
 	return nil
 }
 
@@ -128,7 +177,7 @@ func UpdateNetworkPolicyAttachment(d *schema.ResourceData, meta interface{}) err
 		}
 	}
 
-	return nil
+	return ReadNetworkPolicyAttachment(d, meta)
 }
 
 // DeleteNetworkPolicyAttachment implements schema.DeleteFunc
@@ -136,9 +185,11 @@ func DeleteNetworkPolicyAttachment(d *schema.ResourceData, meta interface{}) err
 	policyName := d.Get("network_policy_name").(string)
 	d.SetId(policyName + "_attachment")
 
-	err := unsetOnAccount(d, meta)
-	if err != nil {
-		return errors.Wrapf(err, "error deleting attachment for network policy %v", policyName)
+	if d.Get("set_for_account").(bool) {
+		err := unsetOnAccount(d, meta)
+		if err != nil {
+			return errors.Wrapf(err, "error deleting attachment for network policy %v", policyName)
+		}
 	}
 
 	if u, ok := d.GetOk("users"); ok {
diff --git a/pkg/snowflake/network_policy.go b/pkg/snowflake/network_policy.go
index 7681e803..6dd6340e 100644
--- a/pkg/snowflake/network_policy.go
+++ b/pkg/snowflake/network_policy.go
@@ -112,6 +112,16 @@ func (npb *NetworkPolicyBuilder) ShowAllNetworkPolicies() string {
 	return `SHOW NETWORK POLICIES`
 }
 
+//ShowOnUser returns the SQL query that will SHOW network policy set on a specific User
+func (npb *NetworkPolicyBuilder) ShowOnUser(u string) string {
+	return fmt.Sprintf(`SHOW PARAMETERS LIKE 'network_policy' IN USER "%v"`, u)
+}
+
+//ShowOnAccount returns the SQL query that will SHOW network policy set on Account
+func (npb *NetworkPolicyBuilder) ShowOnAccount() string {
+	return `SHOW PARAMETERS LIKE 'network_policy' IN ACCOUNT`
+}
+
 // IpListToString formats a list of IPs into a Snowflake-DDL friendly string, e.g. ('192.168.1.0', '192.168.1.100')
 func IpListToString(ips []string) string {
 	for index, element := range ips {
@@ -129,6 +139,12 @@ type NetworkPolicyStruct struct {
 	EntriesInBlockedIpList sql.NullString `db:"entries_in_blocked_ip_list"`
 }
 
+type NetworkPolicyAttachmentStruct struct {
+	Key   sql.NullString `db:"key"`
+	Value sql.NullString `db:"value"`
+	Level sql.NullString `db:"level"`
+}
+
 // ScanNetworkPolicies takes database rows and converts them to a list of NetworkPolicyStruct pointers
 func ScanNetworkPolicies(rows *sqlx.Rows) ([]*NetworkPolicyStruct, error) {
 	var n []*NetworkPolicyStruct
@@ -143,3 +159,9 @@ func ScanNetworkPolicies(rows *sqlx.Rows) ([]*NetworkPolicyStruct, error) {
 	}
 	return n, nil
 }
+
+func ScanNetworkPolicyAttachment(row *sqlx.Row) (*NetworkPolicyAttachmentStruct, error) {
+	r := &NetworkPolicyAttachmentStruct{}
+	err := row.StructScan(r)
+	return r, err
+}


Test patch:
diff --git a/pkg/resources/network_policy_attachment_acceptance_test.go b/pkg/resources/network_policy_attachment_acceptance_test.go
index 82afbf66..c639887b 100644
--- a/pkg/resources/network_policy_attachment_acceptance_test.go
+++ b/pkg/resources/network_policy_attachment_acceptance_test.go
@@ -22,6 +22,14 @@ func TestAcc_NetworkPolicyAttachment(t *testing.T) {
 	resource.ParallelTest(t, resource.TestCase{
 		Providers: providers(),
 		Steps: []resource.TestStep{
+			{
+				Config: networkPolicyAttachmentConfigSingle(user1, policyName),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_network_policy_attachment.test", "network_policy_name", policyName),
+					resource.TestCheckResourceAttr("snowflake_network_policy_attachment.test", "set_for_account", "false"),
+					resource.TestCheckResourceAttr("snowflake_network_policy_attachment.test", "users.#", "1"),
+				),
+			},
 			{
 				Config: networkPolicyAttachmentConfig(user1, user2, policyName),
 				Check: resource.ComposeTestCheckFunc(
@@ -40,6 +48,25 @@ func TestAcc_NetworkPolicyAttachment(t *testing.T) {
 	})
 }
 
+func networkPolicyAttachmentConfigSingle(user1, policyName string) string {
+	return fmt.Sprintf(`
+resource "snowflake_user" "test-user1" {
+	name = "%s"
+}
+
+resource "snowflake_network_policy" "test" {
+	name            = "%v"
+	allowed_ip_list = ["192.168.0.100/24", "29.254.123.20"]
+}
+
+resource "snowflake_network_policy_attachment" "test" {
+	network_policy_name = snowflake_network_policy.test.name
+	set_for_account     = false
+	users               = [snowflake_user.test-user1.name]
+}
+`, user1, policyName)
+}
+
 func networkPolicyAttachmentConfig(user1, user2, policyName string) string {
 	return fmt.Sprintf(`
 resource "snowflake_user" "test-user1" {
diff --git a/pkg/resources/network_policy_attachment_test.go b/pkg/resources/network_policy_attachment_test.go
index 00b8aed7..0cc57d1f 100644
--- a/pkg/resources/network_policy_attachment_test.go
+++ b/pkg/resources/network_policy_attachment_test.go
@@ -39,7 +39,7 @@ func TestNetworkPolicyAttachmentCreate(t *testing.T) {
 	})
 }
 
-func TestNetworkPolicyAttachmentDelete(t *testing.T) {
+func TestNetworkPolicyAttachmentSetOnAccountDelete(t *testing.T) {
 	r := require.New(t)
 
 	in := map[string]interface{}{
@@ -59,3 +59,23 @@ func TestNetworkPolicyAttachmentDelete(t *testing.T) {
 		r.NoError(err)
 	})
 }
+
+func TestNetworkPolicyAttachmentDelete(t *testing.T) {
+	r := require.New(t)
+
+	in := map[string]interface{}{
+		"network_policy_name": "test-network-policy",
+		"set_for_account":     false,
+		"users":               []interface{}{"test-user"},
+	}
+	d := schema.TestResourceDataRaw(t, resources.NetworkPolicyAttachment().Schema, in)
+	r.NotNil(d)
+
+	WithMockDb(t, func(db *sql.DB, mock sqlmock.Sqlmock) {
+		mock.ExpectExec(`^DESCRIBE USER "test-user"$`).WillReturnResult(sqlmock.NewResult(1, 1))
+		mock.ExpectExec(`^ALTER USER "test-user" UNSET NETWORK_POLICY$`).WillReturnResult(sqlmock.NewResult(1, 1))
+
+		err := resources.DeleteNetworkPolicyAttachment(d, db)
+		r.NoError(err)
+	})
+}
diff --git a/pkg/snowflake/network_policy_test.go b/pkg/snowflake/network_policy_test.go
index 8a4e372e..33038272 100644
--- a/pkg/snowflake/network_policy_test.go
+++ b/pkg/snowflake/network_policy_test.go
@@ -121,3 +121,21 @@ func TestNetworkPolicyUnsetOnUser(t *testing.T) {
 	q := s.UnsetOnUser("testuser")
 	r.Equal(`ALTER USER "testuser" UNSET NETWORK_POLICY`, q)
 }
+
+func TestNetworkPolicyShowOnUser(t *testing.T) {
+	r := require.New(t)
+	s := snowflake.NetworkPolicy("test_network_policy")
+	r.NotNil(s)
+
+	q := s.ShowOnUser("testuser")
+	r.Equal(`SHOW PARAMETERS LIKE 'network_policy' IN USER "testuser"`, q)
+}
+
+func TestNetworkPolicyShowOnAccount(t *testing.T) {
+	r := require.New(t)
+	s := snowflake.NetworkPolicy("test_network_policy")
+	r.NotNil(s)
+
+	q := s.ShowOnAccount()
+	r.Equal(`SHOW PARAMETERS LIKE 'network_policy' IN ACCOUNT`, q)
+}