fix: Fix snowflake_share resource not unsetting accounts (#1186)

* Update share.md * Update share.go * Update share_acceptance_test.go * Update resource.tf * Update share_acceptance_test.go * Update integration.yml * Update share.go
Snowflake-Labs · Aug 24, 2022 · 03a225f · 03a225f
1 parent 03a6cb3
commit 03a225f
Show file tree

Hide file tree

Showing 5 changed files with 98 additions and 45 deletions.
diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml
@@ -67,6 +67,8 @@ jobs:
           SNOWFLAKE_PASSWORD: ${{ secrets.SNOWFLAKE_PASSWORD }}
           SNOWFLAKE_ACCOUNT: ${{ secrets.SNOWFLAKE_ACCOUNT }}
           SNOWFLAKE_ROLE: ${{ secrets.SNOWFLAKE_ROLE }}
+          SNOWFLAKE_ACCOUNT_SECOND: ${{ secrets.SNOWFLAKE_ACCOUNT_SECOND }}
+          SNOWFLAKE_ACCOUNT_THIRD: ${{ secrets.SNOWFLAKE_ACCOUNT_THIRD }}
         run: make test-acceptance
 
       - name: find comment

diff --git a/docs/resources/share.md b/docs/resources/share.md
@@ -16,7 +16,7 @@ description: |-
 resource snowflake_share test {
 	name           = "share_name"
 	comment        = "cool comment"
-	accounts       = "organizationName.accountName"
+	accounts       = ["organizationName.accountName"]
 }
 ```
 

diff --git a/examples/resources/snowflake_share/resource.tf b/examples/resources/snowflake_share/resource.tf
@@ -1,5 +1,5 @@
 resource snowflake_share test {
 	name           = "share_name"
 	comment        = "cool comment"
-	accounts       = "organizationName.accountName"
+	accounts       = ["organizationName.accountName"]
 }
diff --git a/pkg/resources/share.go b/pkg/resources/share.go
@@ -90,62 +90,68 @@ func setAccounts(d *schema.ResourceData, meta interface{}) error {
 	name := d.Get("name").(string)
 	accs := expandStringList(d.Get("accounts").([]interface{}))
 
-	if len(accs) > 0 {
-		// There is a race condition where error accounts cannot be added to a
-		// share until after a database is added to the share. Since a database
-		// grant is dependent on the share itself, this is a hack to get the
-		// thing working.
-		// 1. Create new temporary DB
-		tempName := fmt.Sprintf("TEMP_%v_%d", name, time.Now().Unix())
-		tempDB := snowflake.Database(tempName)
-		err := snowflake.Exec(db, tempDB.Create())
-		if err != nil {
-			return errors.Wrapf(err, "error creating temporary DB %v", tempName)
-		}
+	// There is a race condition where error accounts cannot be added to a
+	// share until after a database is added to the share. Since a database
+	// grant is dependent on the share itself, this is a hack to get the
+	// thing working.
+	// 1. Create new temporary DB
+	tempName := fmt.Sprintf("TEMP_%v_%d", name, time.Now().Unix())
+	tempDB := snowflake.Database(tempName)
+	err := snowflake.Exec(db, tempDB.Create())
+	if err != nil {
+		return errors.Wrapf(err, "error creating temporary DB %v", tempName)
+	}
 
-		// 2. Create temporary DB grant to the share
-		tempDBGrant := snowflake.DatabaseGrant(tempName)
-
-		// USAGE can only be granted to one database - granting USAGE on the temp db here
-		// conflicts (and errors) with having a database already shared (i.e. when you
-		// already have a share and are just adding or removing accounts). Instead, use
-		// REFERENCE_USAGE which is intended for multi-database sharing as per Snowflake
-		// documentation here:
-		// https://docs.snowflake.com/en/sql-reference/sql/grant-privilege-share.html#usage-notes
-		// Note however that USAGE will be granted automatically on the temp db for the
-		// case where the main db doesn't already exist, so it will need to be revoked
-		// before deleting the temp db. Where USAGE hasn't been already granted it is not
-		// an error to revoke it, so it's ok to just do the revoke every time.
-		err = snowflake.Exec(db, tempDBGrant.Share(name).Grant("REFERENCE_USAGE", false))
-		if err != nil {
-			return errors.Wrapf(err, "error creating temporary DB REFERENCE_USAGE grant %v", tempName)
-		}
+	// 2. Create temporary DB grant to the share
+	tempDBGrant := snowflake.DatabaseGrant(tempName)
+
+	// USAGE can only be granted to one database - granting USAGE on the temp db here
+	// conflicts (and errors) with having a database already shared (i.e. when you
+	// already have a share and are just adding or removing accounts). Instead, use
+	// REFERENCE_USAGE which is intended for multi-database sharing as per Snowflake
+	// documentation here:
+	// https://docs.snowflake.com/en/sql-reference/sql/grant-privilege-share.html#usage-notes
+	// Note however that USAGE will be granted automatically on the temp db for the
+	// case where the main db doesn't already exist, so it will need to be revoked
+	// before deleting the temp db. Where USAGE hasn't been already granted it is not
+	// an error to revoke it, so it's ok to just do the revoke every time.
+	err = snowflake.Exec(db, tempDBGrant.Share(name).Grant("REFERENCE_USAGE", false))
+	if err != nil {
+		return errors.Wrapf(err, "error creating temporary DB REFERENCE_USAGE grant %v", tempName)
+	}
 
-		// 3. Add the accounts to the share
+	// 3. Add the accounts to the share
+	if len(accs) > 0 {
 		q := fmt.Sprintf(`ALTER SHARE "%v" SET ACCOUNTS=%v`, name, strings.Join(accs, ","))
 		err = snowflake.Exec(db, q)
 		if err != nil {
 			return errors.Wrapf(err, "error adding accounts to share %v", name)
 		}
-
-		// 4. Revoke temporary DB grant to the share
-		err = snowflake.ExecMulti(db, tempDBGrant.Share(name).Revoke("REFERENCE_USAGE"))
+	} else {
+		q := fmt.Sprintf(`ALTER SHARE "%v" UNSET ACCOUNTS`, name)
+		err = snowflake.Exec(db, q)
 		if err != nil {
-			return errors.Wrapf(err, "error revoking temporary DB REFERENCE_USAGE grant %v", tempName)
+			return errors.Wrapf(err, "error unsetting accounts to share %v", name)
 		}
+	}
 
-		// revoke the maybe automatically granted USAGE privilege.
-		err = snowflake.ExecMulti(db, tempDBGrant.Share(name).Revoke("USAGE"))
-		if err != nil {
-			return errors.Wrapf(err, "error revoking temporary DB grant %v", tempName)
-		}
+	// 4. Revoke temporary DB grant to the share
+	err = snowflake.ExecMulti(db, tempDBGrant.Share(name).Revoke("REFERENCE_USAGE"))
+	if err != nil {
+		return errors.Wrapf(err, "error revoking temporary DB REFERENCE_USAGE grant %v", tempName)
+	}
 
-		// 5. Remove the temporary DB
-		err = snowflake.Exec(db, tempDB.Drop())
-		if err != nil {
+	// revoke the maybe automatically granted USAGE privilege.
+	err = snowflake.ExecMulti(db, tempDBGrant.Share(name).Revoke("USAGE"))
+	if err != nil {
+		return errors.Wrapf(err, "error revoking temporary DB grant %v", tempName)
+	}
+
+	// 5. Remove the temporary DB
+	err = snowflake.Exec(db, tempDB.Drop())
+	if err != nil {
 			return errors.Wrapf(err, "error dropping temporary DB %v", tempName)
 		}
-	}
 
 	return nil
 }

diff --git a/pkg/resources/share_acceptance_test.go b/pkg/resources/share_acceptance_test.go
@@ -2,6 +2,7 @@ package resources_test
 
 import (
 	"fmt"
+	"os"
 	"strings"
 	"testing"
 
@@ -15,6 +16,8 @@ const (
 
 func TestAcc_Share(t *testing.T) {
 	name := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
+	account2 := os.Getenv("SNOWFLAKE_ACCOUNT_SECOND")
+	account3 := os.Getenv("SNOWFLAKE_ACCOUNT_THIRD")
 
 	resource.ParallelTest(t, resource.TestCase{
 		Providers:    providers(),
@@ -25,6 +28,28 @@ func TestAcc_Share(t *testing.T) {
 				Check: resource.ComposeTestCheckFunc(
 					resource.TestCheckResourceAttr("snowflake_share.test", "name", name),
 					resource.TestCheckResourceAttr("snowflake_share.test", "comment", shareComment),
+					resource.TestCheckResourceAttr("snowflake_share.test", "accounts.#", "0"),
+				),
+			},
+			{
+				Config: shareConfigTwoAccounts(name, account2, account3),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_share.test", "accounts.#", "2"),
+					resource.TestCheckResourceAttr("snowflake_share.test", "accounts.0", account2),
+					resource.TestCheckResourceAttr("snowflake_share.test", "accounts.1", account3),
+				),
+			},
+			{
+				Config: shareConfigOneAccount(name, account2),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_share.test", "accounts.#", "1"),
+					resource.TestCheckResourceAttr("snowflake_share.test", "accounts.0", account2),
+				),
+			},
+			{
+				Config: shareConfig(name),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_share.test", "accounts.#", "0"),
 				),
 			},
 			// IMPORT
@@ -45,3 +70,23 @@ resource "snowflake_share" "test" {
 }
 `, name, shareComment)
 }
+
+func shareConfigOneAccount(name string, account2 string) string {
+	return fmt.Sprintf(`
+resource "snowflake_share" "test" {
+	name           = "%v"
+	comment        = "%v"
+	accounts       = ["%v"]
+}
+`, name, shareComment, account2)
+}
+
+func shareConfigTwoAccounts(name string, account2 string, account3 string) string {
+	return fmt.Sprintf(`
+resource "snowflake_share" "test" {
+	name           = "%v"
+	comment        = "%v"
+	accounts       = ["%v", "%v"]
+}
+`, name, shareComment, account2, account3)
+}