fix: Fix provider config hierarchy (#2551)

Fix the order of precedence for the provider's config:
- removed fallback to environment variables for the SDK client (we will
address the setup of the client separately with the extraction of the
SDK client from this repository)
- fixed the merge config function to fill out only missing parameters
- added missing tests
- tested the provider setup in an acceptance test
- introduced environment helpers with tests
- started to move env to one place (the rest envs can be moved after
discussing in this PR if we all agree with that direction)
- added migration notes

References: #2242 #2294

MIGRATION_GUIDE.md

@@ -5,6 +5,22 @@ describe deprecations or breaking changes and help you to change your configurat
 across different versions.
 
 ## v0.86.0 ➞ v0.87.0
+### Provider configuration changes
+
+#### **IMPORTANT** *(bug fix)* Configuration hierarchy
+There were several issues reported about the configuration hierarchy, e.g. [#2294](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2294) and [#2242](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2242).
+In fact, the order of precedence described in the docs was not followed. This have led to the incorrect behavior.
+
+After migrating to this version, the hierarchy from the docs should be followed:
+```text
+The Snowflake provider will use the following order of precedence when determining which credentials to use:
+1) Provider Configuration
+2) Environment Variables
+3) Config File
+```
+
+**BEWARE**: your configurations will be affected with that change because they may have been leveraging the incorrect configurations precedence. Please be sure to check all the configurations before running terraform.
+
 ### snowflake_failover_group resource changes
 #### *(bug fix)* ACCOUNT PARAMETERS is returned as PARAMETERS from SHOW FAILOVER GROUPS
 Longer context in [#2517](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2517).

docs/index.md

@@ -7,7 +7,7 @@ description: Manage SnowflakeDB with Terraform.
 
 ~> **Disclaimer** the project is still in the 0.x.x version, which means it’s still in the experimental phase (check [Go module versioning](https://go.dev/doc/modules/version-numbers#v0-number) for more details). It can be used in production but makes no stability or backward compatibility guarantees. We do not provide backward bug fixes and, therefore, always suggest using the newest version. We are providing only limited support for the provider; priorities will be assigned on a case-by-case basis. Our main current goals are stabilization, addressing existing issues, and providing the missing features (prioritizing the GA features; supporting PrPr and PuPr features are not high priorities now). With all that in mind, we aim to reach V1 with a stable, reliable, and functional provider. V1 will be free of all the above limitations.
 
--> **Note** Please check the [migration guide](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md) when changing the version of the provider.
+~> **Note** Please check the [migration guide](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md) when changing the version of the provider.
 
 -> **Note** the current roadmap is available in our GitHub repository: [ROADMAP.md](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md).
 

pkg/acceptance/testenvs/assertions.go

@@ -0,0 +1,18 @@
+package testenvs
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func AssertEnvNotSet(t *testing.T, envName string) {
+	t.Helper()
+	require.Emptyf(t, os.Getenv(envName), "environment variable %v should not be set", envName)
+}
+
+func AssertEnvSet(t *testing.T, envName string) {
+	t.Helper()
+	require.NotEmptyf(t, os.Getenv(envName), "environment variable %v should not be empty", envName)
+}

pkg/acceptance/testenvs/testenvs_test.go

@@ -0,0 +1,86 @@
+package testenvs_test
+
+import (
+	"sync"
+	"testing"
+
+	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testenvs"
+	"github.com/stretchr/testify/require"
+)
+
+func Test_GetOrSkipTest(t *testing.T) {
+	// runGetOrSkipInGoroutineAndWaitForCompletion is needed because underneath we test t.Skipf, that leads to t.SkipNow() that in turn call runtime.Goexit()
+	// so we need to be wrapped in a Goroutine.
+	runGetOrSkipInGoroutineAndWaitForCompletion := func(t *testing.T) string {
+		t.Helper()
+		var env string
+		var wg sync.WaitGroup
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			env = testenvs.GetOrSkipTest(t, testenvs.User)
+		}()
+		wg.Wait()
+		return env
+	}
+
+	t.Run("skip test if missing", func(t *testing.T) {
+		t.Setenv(string(testenvs.User), "")
+
+		tut := &testing.T{}
+		env := runGetOrSkipInGoroutineAndWaitForCompletion(tut)
+
+		require.True(t, tut.Skipped())
+		require.Empty(t, env)
+	})
+
+	t.Run("get env if exists", func(t *testing.T) {
+		t.Setenv(string(testenvs.User), "user")
+
+		tut := &testing.T{}
+		env := runGetOrSkipInGoroutineAndWaitForCompletion(tut)
+
+		require.False(t, tut.Skipped())
+		require.Equal(t, "user", env)
+	})
+}
+
+func Test_Assertions(t *testing.T) {
+	// runAssertionInGoroutineAndWaitForCompletion is needed because underneath we test require, that leads to t.FailNow() that in turn call runtime.Goexit()
+	// so we need to be wrapped in a Goroutine.
+	runAssertionInGoroutineAndWaitForCompletion := func(assertion func()) {
+		var wg sync.WaitGroup
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			assertion()
+		}()
+		wg.Wait()
+	}
+
+	t.Run("test if env does not exist", func(t *testing.T) {
+		t.Setenv(string(testenvs.User), "")
+
+		tut1 := &testing.T{}
+		runAssertionInGoroutineAndWaitForCompletion(func() { testenvs.AssertEnvNotSet(tut1, string(testenvs.User)) })
+
+		tut2 := &testing.T{}
+		runAssertionInGoroutineAndWaitForCompletion(func() { testenvs.AssertEnvSet(tut2, string(testenvs.User)) })
+
+		require.False(t, tut1.Failed())
+		require.True(t, tut2.Failed())
+	})
+
+	t.Run("test if env exists", func(t *testing.T) {
+		t.Setenv(string(testenvs.User), "user")
+
+		tut1 := &testing.T{}
+		runAssertionInGoroutineAndWaitForCompletion(func() { testenvs.AssertEnvNotSet(tut1, string(testenvs.User)) })
+
+		tut2 := &testing.T{}
+		runAssertionInGoroutineAndWaitForCompletion(func() { testenvs.AssertEnvSet(tut2, string(testenvs.User)) })
+
+		require.True(t, tut1.Failed())
+		require.False(t, tut2.Failed())
+	})
+}

pkg/acceptance/testenvs/testing_environment_variables.go

@@ -0,0 +1,32 @@
+package testenvs
+
+import (
+	"fmt"
+	"os"
+	"testing"
+)
+
+type env string
+
+const (
+	User     env = "TEST_SF_TF_USER"
+	Password env = "TEST_SF_TF_PASSWORD" // #nosec G101
+	Account  env = "TEST_SF_TF_ACCOUNT"
+	Role     env = "TEST_SF_TF_ROLE"
+	Host     env = "TEST_SF_TF_HOST"
+)
+
+func GetOrSkipTest(t *testing.T, envName Env) string {
+	t.Helper()
+	env := os.Getenv(fmt.Sprintf("%v", envName))
+	if env == "" {
+		t.Skipf("Skipping %s, env %v missing", t.Name(), envName)
+	}
+	return env
+}
+
+type Env interface {
+	xxxProtected()
+}
+
+func (e env) xxxProtected() {}

pkg/acceptance/testprofiles/testing_config_profiles.go

@@ -0,0 +1,7 @@
+package testprofiles
+
+const (
+	Default                  = "default"
+	Secondary                = "secondary_test_account"
+	IncorrectUserAndPassword = "incorrect_test_profile"
+)

pkg/internal/snowflakeenvs/snowflake_environment_variables.go

@@ -0,0 +1,10 @@
+package snowflakeenvs
+
+const (
+	Account    = "SNOWFLAKE_ACCOUNT"
+	User       = "SNOWFLAKE_USER"
+	Password   = "SNOWFLAKE_PASSWORD"
+	Role       = "SNOWFLAKE_ROLE"
+	ConfigPath = "SNOWFLAKE_CONFIG_PATH"
+	Host       = "SNOWFLAKE_HOST"
+)

pkg/provider/provider_acceptance_test.go

@@ -0,0 +1,160 @@
+package provider_test
+
+import (
+	"fmt"
+	"os"
+	"regexp"
+	"testing"
+
+	acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance"
+
+	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testenvs"
+	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testprofiles"
+	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/snowflakeenvs"
+	"github.com/hashicorp/terraform-plugin-testing/helper/resource"
+	"github.com/hashicorp/terraform-plugin-testing/tfversion"
+	"github.com/stretchr/testify/require"
+)
+
+func TestAcc_Provider_configHierarchy(t *testing.T) {
+	user := testenvs.GetOrSkipTest(t, testenvs.User)
+	pass := testenvs.GetOrSkipTest(t, testenvs.Password)
+	account := testenvs.GetOrSkipTest(t, testenvs.Account)
+	role := testenvs.GetOrSkipTest(t, testenvs.Role)
+	host := testenvs.GetOrSkipTest(t, testenvs.Host)
+
+	nonExistingUser := "non-existing-user"
+
+	resource.Test(t, resource.TestCase{
+		ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+		PreCheck: func() {
+			acc.TestAccPreCheck(t)
+			testenvs.AssertEnvNotSet(t, snowflakeenvs.User)
+			testenvs.AssertEnvNotSet(t, snowflakeenvs.Password)
+		},
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.RequireAbove(tfversion.Version1_5_0),
+		},
+		Steps: []resource.TestStep{
+			// make sure that we fail for incorrect profile
+			{
+				Config:      providerConfig(testprofiles.IncorrectUserAndPassword),
+				ExpectError: regexp.MustCompile("Incorrect username or password was specified"),
+			},
+			// incorrect user in provider config should not be rewritten by profile and cause error
+			{
+				Config:      providerConfigWithUser(nonExistingUser, testprofiles.Default),
+				ExpectError: regexp.MustCompile("Incorrect username or password was specified"),
+			},
+			// correct user and password in provider's config should not be rewritten by a faulty config
+			{
+				Config: providerConfigWithUserAndPassword(user, pass, testprofiles.IncorrectUserAndPassword),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("data.snowflake_database.t", "name", acc.TestDatabaseName),
+				),
+			},
+			// incorrect user in env variable should not be rewritten by profile and cause error
+			{
+				PreConfig: func() {
+					t.Setenv(snowflakeenvs.User, nonExistingUser)
+				},
+				Config:      providerConfig(testprofiles.Default),
+				ExpectError: regexp.MustCompile("Incorrect username or password was specified"),
+			},
+			// correct user and password in env should not be rewritten by a faulty config
+			{
+				PreConfig: func() {
+					t.Setenv(snowflakeenvs.User, user)
+					t.Setenv(snowflakeenvs.Password, pass)
+				},
+				Config: providerConfig(testprofiles.IncorrectUserAndPassword),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("data.snowflake_database.t", "name", acc.TestDatabaseName),
+				),
+			},
+			// user on provider level wins (it's incorrect - env and profile ones are)
+			{
+				Config:      providerConfigWithUser(nonExistingUser, testprofiles.Default),
+				ExpectError: regexp.MustCompile("Incorrect username or password was specified"),
+			},
+			// there is no config (by setting the dir to something different than .snowflake/config)
+			{
+				PreConfig: func() {
+					dir, err := os.UserHomeDir()
+					require.NoError(t, err)
+					t.Setenv(snowflakeenvs.ConfigPath, dir)
+				},
+				Config:      providerConfigWithUserAndPassword(user, pass, testprofiles.Default),
+				ExpectError: regexp.MustCompile("account is empty"),
+			},
+			// provider's config should not be rewritten by env when there is no profile (incorrect user in config versus correct one in env) - proves #2242
+			{
+				PreConfig: func() {
+					testenvs.AssertEnvSet(t, snowflakeenvs.ConfigPath)
+					t.Setenv(snowflakeenvs.User, user)
+					t.Setenv(snowflakeenvs.Password, pass)
+					t.Setenv(snowflakeenvs.Account, account)
+					t.Setenv(snowflakeenvs.Role, role)
+					t.Setenv(snowflakeenvs.Host, host)
+				},
+				Config:      providerConfigWithUser(nonExistingUser, testprofiles.Default),
+				ExpectError: regexp.MustCompile("Incorrect username or password was specified"),
+			},
+			// make sure the teardown is fine by using a correct env config at the end
+			{
+				PreConfig: func() {
+					testenvs.AssertEnvSet(t, snowflakeenvs.ConfigPath)
+					testenvs.AssertEnvSet(t, snowflakeenvs.User)
+					testenvs.AssertEnvSet(t, snowflakeenvs.Password)
+					testenvs.AssertEnvSet(t, snowflakeenvs.Account)
+					testenvs.AssertEnvSet(t, snowflakeenvs.Role)
+					testenvs.AssertEnvSet(t, snowflakeenvs.Host)
+				},
+				Config: emptyProviderConfig(),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("data.snowflake_database.t", "name", acc.TestDatabaseName),
+				),
+			},
+		},
+	})
+}
+
+func emptyProviderConfig() string {
+	return `
+provider "snowflake" {
+}` + datasourceConfig()
+}
+
+func providerConfig(profile string) string {
+	return fmt.Sprintf(`
+provider "snowflake" {
+	profile = "%[1]s"
+}
+`, profile) + datasourceConfig()
+}
+
+func providerConfigWithUser(user string, profile string) string {
+	return fmt.Sprintf(`
+provider "snowflake" {
+	user = "%[1]s"
+	profile = "%[2]s"
+}
+`, user, profile) + datasourceConfig()
+}
+
+func providerConfigWithUserAndPassword(user string, pass string, profile string) string {
+	return fmt.Sprintf(`
+provider "snowflake" {
+	user = "%[1]s"
+	password = "%[2]s"
+	profile = "%[3]s"
+}
+`, user, pass, profile) + datasourceConfig()
+}
+
+func datasourceConfig() string {
+	return fmt.Sprintf(`
+data snowflake_database "t" {
+	name = "%s"
+}`, acc.TestDatabaseName)
+}

pkg/resources/database_acceptance_test.go

@@ -8,6 +8,7 @@ import (
 
 	acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance"
 
+	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testprofiles"
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
 	"github.com/hashicorp/terraform-plugin-testing/config"
 	"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
@@ -228,7 +229,7 @@ func dropDatabaseOutsideTerraform(t *testing.T, id string) {
 func getSecondaryAccount(t *testing.T) string {
 	t.Helper()
 
-	secondaryConfig, err := sdk.ProfileConfig("secondary_test_account")
+	secondaryConfig, err := sdk.ProfileConfig(testprofiles.Secondary)
 	require.NoError(t, err)
 
 	secondaryClient, err := sdk.NewClient(secondaryConfig)