From c4d76cb62803de5b0d743fdb1219001be1769d58 Mon Sep 17 00:00:00 2001 From: Jakub Michalak Date: Mon, 10 Mar 2025 15:03:35 +0100 Subject: [PATCH 1/4] Limit TOML file size --- MIGRATION_GUIDE.md | 5 ++++ .../helpers/tmp_toml_setup_helpers.go | 8 ++++++ pkg/manual_tests/README.md | 1 + pkg/manual_tests/windows/files_test.go | 21 ++++++++++++++++ pkg/provider/provider_acceptance_test.go | 25 +++++++++++++++++++ pkg/sdk/config.go | 24 ++++++++++++++++-- pkg/sdk/config_test.go | 23 ++++++++++++++--- templates/index.md.tmpl | 2 ++ 8 files changed, 104 insertions(+), 5 deletions(-) create mode 100644 pkg/manual_tests/windows/files_test.go diff --git a/MIGRATION_GUIDE.md b/MIGRATION_GUIDE.md index d07dec31ff9..a4aecf2756e 100644 --- a/MIGRATION_GUIDE.md +++ b/MIGRATION_GUIDE.md @@ -9,6 +9,11 @@ across different versions. ## v1.0.3 ➞ v1.0.4 +### Changes in TOML configuration file requirements +Before this version, it was possible to abuse the provider by providing a huge TOML config file which was read every time. To mitigate this, we set a limit of the supported file size to 10MB. + +## v1.0.3 ➞ v1.0.4 + ### Fixed external_function VARCHAR return_type VARCHAR external_function return_type did not work correctly before ([#3392](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/3392)) but was fixed in this version. diff --git a/pkg/acceptance/helpers/tmp_toml_setup_helpers.go b/pkg/acceptance/helpers/tmp_toml_setup_helpers.go index 6d4fc701ae2..b791d467800 100644 --- a/pkg/acceptance/helpers/tmp_toml_setup_helpers.go +++ b/pkg/acceptance/helpers/tmp_toml_setup_helpers.go @@ -51,6 +51,14 @@ func (c *TestClient) TempIncorrectTomlConfigForLegacyServiceUser(t *testing.T, l }) } +func (c *TestClient) TempTooBigTomlConfigForServiceUser(t *testing.T, serviceUser *TmpServiceUser) *TmpTomlConfig { + t.Helper() + return c.StoreTempTomlConfig(t, func(profile string) string { + c := make([]byte, 11*1024*1024) + return TomlConfigForServiceUser(t, profile, serviceUser.UserId, serviceUser.RoleId, serviceUser.WarehouseId, serviceUser.AccountId, string(c)) + }) +} + func (c *TestClient) StoreTempTomlConfig(t *testing.T, tomlProvider func(string) string) *TmpTomlConfig { t.Helper() diff --git a/pkg/manual_tests/README.md b/pkg/manual_tests/README.md index a0b4d986068..c33347498f8 100644 --- a/pkg/manual_tests/README.md +++ b/pkg/manual_tests/README.md @@ -8,3 +8,4 @@ Here's the list of cases we currently cannot reproduce and write acceptance test - `user_default_database_and_role`: Setting up a user with default_namespace and default_role, then logging into that user to see what happens with those values in various scenarios (e.g. insufficient privileges on the role). - `authentication_methods`: Some of the authentication methods require manual steps, like confirming MFA or setting more dependencies. - `benchmarks`: Performance benchmarks require manually running `terraform` command to imitate the user workflow. +- `windows`: Test cases for the Windows platform. Potentially, these tests could be incorporated in a platform-specific testing workflows. diff --git a/pkg/manual_tests/windows/files_test.go b/pkg/manual_tests/windows/files_test.go new file mode 100644 index 00000000000..9aee1566c4f --- /dev/null +++ b/pkg/manual_tests/windows/files_test.go @@ -0,0 +1,21 @@ +package windows_test + +import ( + "fmt" + "testing" + + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/testhelpers" + "github.com/stretchr/testify/require" +) + +func TestLoadConfigFileThatIsTooBig(t *testing.T) { + if !sdk.IsRunningOnWindows { + t.Skip("checking file sizes on other platforms is currently done in the sdk package") + } + c := make([]byte, 11*1024*1024) + configPath := testhelpers.TestFile(t, "config", c) + + _, err := sdk.LoadConfigFile(configPath) + require.ErrorContains(t, err, fmt.Sprintf("config file %s is too big - maximum allowed size is 10MB", configPath)) +} diff --git a/pkg/provider/provider_acceptance_test.go b/pkg/provider/provider_acceptance_test.go index 15068bc9f23..c30aee9e55c 100644 --- a/pkg/provider/provider_acceptance_test.go +++ b/pkg/provider/provider_acceptance_test.go @@ -296,6 +296,31 @@ func TestAcc_Provider_tomlConfig(t *testing.T) { }) } +func TestAcc_Provider_tomlConfigIsTooBig(t *testing.T) { + _ = testenvs.GetOrSkipTest(t, testenvs.EnableAcceptance) + acc.TestAccPreCheck(t) + t.Setenv(string(testenvs.ConfigureClientOnce), "") + + tmpServiceUser := acc.TestClient().SetUpTemporaryServiceUser(t) + tmpServiceUserConfig := acc.TestClient().TempTooBigTomlConfigForServiceUser(t, tmpServiceUser) + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + PreConfig: func() { + t.Setenv(snowflakeenvs.ConfigPath, tmpServiceUserConfig.Path) + }, + Config: config.FromModels(t, providermodel.SnowflakeProvider().WithProfile(tmpServiceUserConfig.Profile), datasourceModel()), + ExpectError: regexp.MustCompile(fmt.Sprintf("could not load config file: config file %s is too big - maximum allowed size is 10MB", tmpServiceUserConfig.Path)), + }, + }, + }) +} + func TestAcc_Provider_envConfig(t *testing.T) { _ = testenvs.GetOrSkipTest(t, testenvs.EnableAcceptance) acc.TestAccPreCheck(t) diff --git a/pkg/sdk/config.go b/pkg/sdk/config.go index c27e4db5c76..50237cdff77 100644 --- a/pkg/sdk/config.go +++ b/pkg/sdk/config.go @@ -10,6 +10,7 @@ import ( "net/url" "os" "path/filepath" + "runtime" "slices" "strings" "time" @@ -21,6 +22,11 @@ import ( "golang.org/x/crypto/ssh" ) +const ( + maxFileSizeInMb = 10 + IsRunningOnWindows = runtime.GOOS == "windows" +) + func DefaultConfig() *gosnowflake.Config { config, err := ProfileConfig("default") if err != nil || config == nil { @@ -36,7 +42,7 @@ func ProfileConfig(profile string) (*gosnowflake.Config, error) { return nil, err } - configs, err := loadConfigFile(path) + configs, err := LoadConfigFile(path) if err != nil { return nil, fmt.Errorf("could not load config file: %w", err) } @@ -360,7 +366,10 @@ func pointerUrlAttributeSet(src *string, dst **url.URL) error { return nil } -func loadConfigFile(path string) (map[string]ConfigDTO, error) { +func LoadConfigFile(path string) (map[string]ConfigDTO, error) { + if err := validateFile(path); err != nil { + return nil, err + } dat, err := os.ReadFile(path) if err != nil { return nil, err @@ -373,6 +382,17 @@ func loadConfigFile(path string) (map[string]ConfigDTO, error) { return s, nil } +func validateFile(path string) error { + fileinfo, err := os.Stat(path) + if err != nil { + return fmt.Errorf("could not read information of the config file: %w", err) + } + if fileinfo.Size() > maxFileSizeInMb*1024*1024 { + return fmt.Errorf("config file %s is too big - maximum allowed size is %dMB", path, maxFileSizeInMb) + } + return nil +} + func ParsePrivateKey(privateKeyBytes []byte, passphrase []byte) (*rsa.PrivateKey, error) { privateKeyBlock, _ := pem.Decode(privateKeyBytes) if privateKeyBlock == nil { diff --git a/pkg/sdk/config_test.go b/pkg/sdk/config_test.go index 2cb49375079..60804ec5659 100644 --- a/pkg/sdk/config_test.go +++ b/pkg/sdk/config_test.go @@ -36,7 +36,7 @@ func TestLoadConfigFile(t *testing.T) { ` configPath := testhelpers.TestFile(t, "config", []byte(c)) - m, err := loadConfigFile(configPath) + m, err := LoadConfigFile(configPath) require.NoError(t, err) assert.Equal(t, "TEST_ACCOUNT", *m["default"].AccountName) assert.Equal(t, "TEST_ORG", *m["default"].OrganizationName) @@ -58,7 +58,7 @@ func TestLoadConfigFileWithUnknownFields(t *testing.T) { ` configPath := testhelpers.TestFile(t, "config", []byte(c)) - m, err := loadConfigFile(configPath) + m, err := LoadConfigFile(configPath) require.NoError(t, err) assert.Equal(t, map[string]ConfigDTO{ "default": { @@ -74,10 +74,27 @@ func TestLoadConfigFileWithInvalidFieldValue(t *testing.T) { ` configPath := testhelpers.TestFile(t, "config", []byte(c)) - _, err := loadConfigFile(configPath) + _, err := LoadConfigFile(configPath) require.ErrorContains(t, err, "toml: cannot decode TOML integer into struct field sdk.ConfigDTO.AccountName of type *string") } +func TestLoadConfigFileThatIsTooBig(t *testing.T) { + if IsRunningOnWindows { + t.Skip("checking file sizes on Windows is currently done in manual tests package") + } + c := make([]byte, 11*1024*1024) + configPath := testhelpers.TestFile(t, "config", c) + + _, err := LoadConfigFile(configPath) + require.ErrorContains(t, err, fmt.Sprintf("config file %s is too big - maximum allowed size is 10MB", configPath)) +} + +func TestLoadConfigFileThatDoesNotExist(t *testing.T) { + configPath := "non-existing" + _, err := LoadConfigFile(configPath) + require.ErrorContains(t, err, fmt.Sprintf("could not read information of the config file: stat %s: no such file or directory", configPath)) +} + func TestProfileConfig(t *testing.T) { unencryptedKey, encryptedKey := random.GenerateRSAPrivateKeyEncrypted(t, "password") diff --git a/templates/index.md.tmpl b/templates/index.md.tmpl index dad4298d1ce..54faeb524cf 100644 --- a/templates/index.md.tmpl +++ b/templates/index.md.tmpl @@ -172,6 +172,8 @@ password='password' role='ACCOUNTADMIN' ``` +--> **Note: TOML file size is limited to 10MB. + Not all fields must be configured in one source; users can choose which fields are configured in which source. Provider uses an established hierarchy of sources. The current behavior is that for each field: 1. Check if it is present in the provider configuration. If yes, use this value. If not, go to step 2. From fa5a7e0a5c8f7fc5e68e2c766e95960eb9cbcd29 Mon Sep 17 00:00:00 2001 From: Jakub Michalak Date: Mon, 10 Mar 2025 16:23:12 +0100 Subject: [PATCH 2/4] Move code to internal/os --- MIGRATION_GUIDE.md | 2 +- docs/index.md | 2 + pkg/internal/logging/debug_helpers.go | 7 +++- pkg/internal/os/os.go | 57 ++++++++++++++++++++++++++ pkg/internal/os/os_test.go | 27 ++++++++++++ pkg/manual_tests/windows/files_test.go | 3 +- pkg/provider/provider.go | 2 +- pkg/sdk/client.go | 5 +-- pkg/sdk/config.go | 24 +---------- pkg/sdk/config_test.go | 17 -------- 10 files changed, 99 insertions(+), 47 deletions(-) create mode 100644 pkg/internal/os/os.go create mode 100644 pkg/internal/os/os_test.go diff --git a/MIGRATION_GUIDE.md b/MIGRATION_GUIDE.md index a4aecf2756e..ced080abe58 100644 --- a/MIGRATION_GUIDE.md +++ b/MIGRATION_GUIDE.md @@ -7,7 +7,7 @@ across different versions. > [!TIP] > We highly recommend upgrading the versions one by one instead of bulk upgrades. -## v1.0.3 ➞ v1.0.4 +## v1.0.4 ➞ v1.0.5 ### Changes in TOML configuration file requirements Before this version, it was possible to abuse the provider by providing a huge TOML config file which was read every time. To mitigate this, we set a limit of the supported file size to 10MB. diff --git a/docs/index.md b/docs/index.md index aa4f22c615c..3685a7a53f3 100644 --- a/docs/index.md +++ b/docs/index.md @@ -273,6 +273,8 @@ password='password' role='ACCOUNTADMIN' ``` +--> **Note: TOML file size is limited to 10MB. + Not all fields must be configured in one source; users can choose which fields are configured in which source. Provider uses an established hierarchy of sources. The current behavior is that for each field: 1. Check if it is present in the provider configuration. If yes, use this value. If not, go to step 2. diff --git a/pkg/internal/logging/debug_helpers.go b/pkg/internal/logging/debug_helpers.go index c8f6e507082..902baf6f308 100644 --- a/pkg/internal/logging/debug_helpers.go +++ b/pkg/internal/logging/debug_helpers.go @@ -3,12 +3,15 @@ package logging import ( "io" "log" - "os" + pkgos "os" + + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/os" ) +// TODO (next PRs): remove extra logging func init() { additionalDebugLoggingEnabled = os.Getenv("SF_TF_ADDITIONAL_DEBUG_LOGGING") != "" - DebugLogger = log.New(os.Stderr, "sf-tf-additional-debug ", log.LstdFlags|log.Lmsgprefix|log.LUTC|log.Lmicroseconds) + DebugLogger = log.New(pkgos.Stderr, "sf-tf-additional-debug ", log.LstdFlags|log.Lmsgprefix|log.LUTC|log.Lmicroseconds) if !additionalDebugLoggingEnabled { DebugLogger.SetOutput(io.Discard) } diff --git a/pkg/internal/os/os.go b/pkg/internal/os/os.go new file mode 100644 index 00000000000..6e7b539704a --- /dev/null +++ b/pkg/internal/os/os.go @@ -0,0 +1,57 @@ +package os + +import ( + "fmt" + "log" + "os" + "runtime" +) + +const ( + IsRunningOnWindows = runtime.GOOS == "windows" + maxFileSizeInMb = 10 +) + +func Stat(path string) (os.FileInfo, error) { + log.Printf("[DEBUG] reading the %s file info", path) + return os.Stat(path) +} + +func Getenv(name string) string { + log.Printf("[DEBUG] reading the %s environmental variable", name) + return os.Getenv(name) +} + +func LookupEnv(name string) (string, bool) { + log.Printf("[DEBUG] reading the %s environmental variable", name) + return os.LookupEnv(name) +} + +func ReadFile(path string) ([]byte, error) { + log.Printf("[DEBUG] reading the %s file", path) + return os.ReadFile(path) +} + +// ReadFileSafe checks if a file is safe to read, and then reads it. +func ReadFileSafe(path string) ([]byte, error) { + if err := validateFile(path); err != nil { + return nil, err + } + return ReadFile(path) +} + +func validateFile(path string) error { + fileinfo, err := Stat(path) + if err != nil { + return fmt.Errorf("could not read information of the config file: %w", err) + } + if fileinfo.Size() > maxFileSizeInMb*1024*1024 { + return fmt.Errorf("config file %s is too big - maximum allowed size is %dMB", path, maxFileSizeInMb) + } + return nil +} + +func UserHomeDir() (string, error) { + log.Printf("[DEBUG] reading the user home directory location from the operating system") + return os.UserHomeDir() +} diff --git a/pkg/internal/os/os_test.go b/pkg/internal/os/os_test.go new file mode 100644 index 00000000000..0b141477580 --- /dev/null +++ b/pkg/internal/os/os_test.go @@ -0,0 +1,27 @@ +package os_test + +import ( + "fmt" + "testing" + + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/os" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/testhelpers" + "github.com/stretchr/testify/require" +) + +func TestLoadConfigFileThatIsTooBig(t *testing.T) { + if os.IsRunningOnWindows { + t.Skip("checking file sizes on Windows is currently done in manual tests package") + } + c := make([]byte, 11*1024*1024) + configPath := testhelpers.TestFile(t, "config", c) + + _, err := os.ReadFileSafe(configPath) + require.ErrorContains(t, err, fmt.Sprintf("config file %s is too big - maximum allowed size is 10MB", configPath)) +} + +func TestLoadConfigFileThatDoesNotExist(t *testing.T) { + configPath := "non-existing" + _, err := os.ReadFileSafe(configPath) + require.ErrorContains(t, err, fmt.Sprintf("could not read information of the config file: stat %s: no such file or directory", configPath)) +} diff --git a/pkg/manual_tests/windows/files_test.go b/pkg/manual_tests/windows/files_test.go index 9aee1566c4f..f10b880b687 100644 --- a/pkg/manual_tests/windows/files_test.go +++ b/pkg/manual_tests/windows/files_test.go @@ -4,13 +4,14 @@ import ( "fmt" "testing" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/os" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/testhelpers" "github.com/stretchr/testify/require" ) func TestLoadConfigFileThatIsTooBig(t *testing.T) { - if !sdk.IsRunningOnWindows { + if !os.IsRunningOnWindows { t.Skip("checking file sizes on other platforms is currently done in the sdk package") } c := make([]byte, 11*1024*1024) diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 7d79f6fbe06..3351d5071c6 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -6,11 +6,11 @@ import ( "fmt" "net" "net/url" - "os" "strings" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testenvs" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/datasources" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/os" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider/docs" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider/validators" diff --git a/pkg/sdk/client.go b/pkg/sdk/client.go index 240162c2f1a..0119edf6df8 100644 --- a/pkg/sdk/client.go +++ b/pkg/sdk/client.go @@ -5,12 +5,11 @@ import ( "database/sql" "fmt" "log" - "os" "slices" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/tracking" - + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/os" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/snowflakeenvs" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/tracking" "github.com/jmoiron/sqlx" "github.com/luna-duclos/instrumentedsql" "github.com/snowflakedb/gosnowflake" diff --git a/pkg/sdk/config.go b/pkg/sdk/config.go index 50237cdff77..cf1fc628e77 100644 --- a/pkg/sdk/config.go +++ b/pkg/sdk/config.go @@ -8,25 +8,19 @@ import ( "log" "net" "net/url" - "os" "path/filepath" - "runtime" "slices" "strings" "time" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/collections" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/os" "github.com/pelletier/go-toml/v2" "github.com/snowflakedb/gosnowflake" "github.com/youmark/pkcs8" "golang.org/x/crypto/ssh" ) -const ( - maxFileSizeInMb = 10 - IsRunningOnWindows = runtime.GOOS == "windows" -) - func DefaultConfig() *gosnowflake.Config { config, err := ProfileConfig("default") if err != nil || config == nil { @@ -367,10 +361,7 @@ func pointerUrlAttributeSet(src *string, dst **url.URL) error { } func LoadConfigFile(path string) (map[string]ConfigDTO, error) { - if err := validateFile(path); err != nil { - return nil, err - } - dat, err := os.ReadFile(path) + dat, err := os.ReadFileSafe(path) if err != nil { return nil, err } @@ -382,17 +373,6 @@ func LoadConfigFile(path string) (map[string]ConfigDTO, error) { return s, nil } -func validateFile(path string) error { - fileinfo, err := os.Stat(path) - if err != nil { - return fmt.Errorf("could not read information of the config file: %w", err) - } - if fileinfo.Size() > maxFileSizeInMb*1024*1024 { - return fmt.Errorf("config file %s is too big - maximum allowed size is %dMB", path, maxFileSizeInMb) - } - return nil -} - func ParsePrivateKey(privateKeyBytes []byte, passphrase []byte) (*rsa.PrivateKey, error) { privateKeyBlock, _ := pem.Decode(privateKeyBytes) if privateKeyBlock == nil { diff --git a/pkg/sdk/config_test.go b/pkg/sdk/config_test.go index 60804ec5659..9e04845d43f 100644 --- a/pkg/sdk/config_test.go +++ b/pkg/sdk/config_test.go @@ -78,23 +78,6 @@ func TestLoadConfigFileWithInvalidFieldValue(t *testing.T) { require.ErrorContains(t, err, "toml: cannot decode TOML integer into struct field sdk.ConfigDTO.AccountName of type *string") } -func TestLoadConfigFileThatIsTooBig(t *testing.T) { - if IsRunningOnWindows { - t.Skip("checking file sizes on Windows is currently done in manual tests package") - } - c := make([]byte, 11*1024*1024) - configPath := testhelpers.TestFile(t, "config", c) - - _, err := LoadConfigFile(configPath) - require.ErrorContains(t, err, fmt.Sprintf("config file %s is too big - maximum allowed size is 10MB", configPath)) -} - -func TestLoadConfigFileThatDoesNotExist(t *testing.T) { - configPath := "non-existing" - _, err := LoadConfigFile(configPath) - require.ErrorContains(t, err, fmt.Sprintf("could not read information of the config file: stat %s: no such file or directory", configPath)) -} - func TestProfileConfig(t *testing.T) { unencryptedKey, encryptedKey := random.GenerateRSAPrivateKeyEncrypted(t, "password") From 2fe017070d18c0ded84fbb424eca9da29a2f938a Mon Sep 17 00:00:00 2001 From: Jakub Michalak Date: Tue, 11 Mar 2025 10:15:27 +0100 Subject: [PATCH 3/4] Fix --- pkg/internal/os/os.go | 14 +++++++------- pkg/internal/os/os_test.go | 2 +- pkg/sdk/config_test.go | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/internal/os/os.go b/pkg/internal/os/os.go index 6e7b539704a..c580d03bc7d 100644 --- a/pkg/internal/os/os.go +++ b/pkg/internal/os/os.go @@ -27,23 +27,23 @@ func LookupEnv(name string) (string, bool) { return os.LookupEnv(name) } -func ReadFile(path string) ([]byte, error) { - log.Printf("[DEBUG] reading the %s file", path) - return os.ReadFile(path) -} - // ReadFileSafe checks if a file is safe to read, and then reads it. func ReadFileSafe(path string) ([]byte, error) { if err := validateFile(path); err != nil { return nil, err } - return ReadFile(path) + return readFile(path) +} + +func readFile(path string) ([]byte, error) { + log.Printf("[DEBUG] reading the %s file", path) + return os.ReadFile(path) } func validateFile(path string) error { fileinfo, err := Stat(path) if err != nil { - return fmt.Errorf("could not read information of the config file: %w", err) + return fmt.Errorf("reading information about the config file: %w", err) } if fileinfo.Size() > maxFileSizeInMb*1024*1024 { return fmt.Errorf("config file %s is too big - maximum allowed size is %dMB", path, maxFileSizeInMb) diff --git a/pkg/internal/os/os_test.go b/pkg/internal/os/os_test.go index 0b141477580..44cac2d0e88 100644 --- a/pkg/internal/os/os_test.go +++ b/pkg/internal/os/os_test.go @@ -23,5 +23,5 @@ func TestLoadConfigFileThatIsTooBig(t *testing.T) { func TestLoadConfigFileThatDoesNotExist(t *testing.T) { configPath := "non-existing" _, err := os.ReadFileSafe(configPath) - require.ErrorContains(t, err, fmt.Sprintf("could not read information of the config file: stat %s: no such file or directory", configPath)) + require.ErrorContains(t, err, fmt.Sprintf("reading information about the config file: stat %s: no such file or directory", configPath)) } diff --git a/pkg/sdk/config_test.go b/pkg/sdk/config_test.go index 9e04845d43f..c9e2d0d429c 100644 --- a/pkg/sdk/config_test.go +++ b/pkg/sdk/config_test.go @@ -193,7 +193,7 @@ func TestProfileConfig(t *testing.T) { t.Setenv(snowflakeenvs.ConfigPath, filename) config, err := ProfileConfig("orgadmin") - require.ErrorContains(t, err, fmt.Sprintf("could not load config file: open %s: no such file or directory", filename)) + require.ErrorContains(t, err, fmt.Sprintf("could not load config file: reading information about the config file: stat %s: no such file or directory", filename)) require.Nil(t, config) }) } From 3fd2d3ae861504138c61827de1134b4eb21ade72 Mon Sep 17 00:00:00 2001 From: Jakub Michalak Date: Wed, 12 Mar 2025 14:29:47 +0100 Subject: [PATCH 4/4] Review --- pkg/internal/logging/debug_helpers.go | 8 ++++---- pkg/internal/{os => oswrapper}/os.go | 20 +++++++++++++++----- pkg/internal/{os => oswrapper}/os_test.go | 10 +++++----- pkg/manual_tests/windows/files_test.go | 4 ++-- pkg/provider/provider.go | 10 +++++----- pkg/provider/provider_acceptance_test.go | 12 +++++++----- pkg/sdk/config.go | 8 ++++---- 7 files changed, 42 insertions(+), 30 deletions(-) rename pkg/internal/{os => oswrapper}/os.go (65%) rename pkg/internal/{os => oswrapper}/os_test.go (82%) diff --git a/pkg/internal/logging/debug_helpers.go b/pkg/internal/logging/debug_helpers.go index 902baf6f308..b9b7557052b 100644 --- a/pkg/internal/logging/debug_helpers.go +++ b/pkg/internal/logging/debug_helpers.go @@ -3,15 +3,15 @@ package logging import ( "io" "log" - pkgos "os" + "os" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/os" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/oswrapper" ) // TODO (next PRs): remove extra logging func init() { - additionalDebugLoggingEnabled = os.Getenv("SF_TF_ADDITIONAL_DEBUG_LOGGING") != "" - DebugLogger = log.New(pkgos.Stderr, "sf-tf-additional-debug ", log.LstdFlags|log.Lmsgprefix|log.LUTC|log.Lmicroseconds) + additionalDebugLoggingEnabled = oswrapper.Getenv("SF_TF_ADDITIONAL_DEBUG_LOGGING") != "" + DebugLogger = log.New(os.Stderr, "sf-tf-additional-debug ", log.LstdFlags|log.Lmsgprefix|log.LUTC|log.Lmicroseconds) if !additionalDebugLoggingEnabled { DebugLogger.SetOutput(io.Discard) } diff --git a/pkg/internal/os/os.go b/pkg/internal/oswrapper/os.go similarity index 65% rename from pkg/internal/os/os.go rename to pkg/internal/oswrapper/os.go index c580d03bc7d..adc4813bef4 100644 --- a/pkg/internal/os/os.go +++ b/pkg/internal/oswrapper/os.go @@ -1,4 +1,6 @@ -package os +// Package oswrapper is a wrapper around the standard os package that allows more secure interactions with the operating system. +// It should be used as a replacement in production code of the standard os package. +package oswrapper import ( "fmt" @@ -8,20 +10,27 @@ import ( ) const ( - IsRunningOnWindows = runtime.GOOS == "windows" - maxFileSizeInMb = 10 + maxFileSizeInMb = 10 ) +// IsRunningOnWindows returns true if the code is running on Windows. +func IsRunningOnWindows() bool { + return runtime.GOOS == "windows" +} + +// Stat is an os.Stat wrapper. func Stat(path string) (os.FileInfo, error) { log.Printf("[DEBUG] reading the %s file info", path) return os.Stat(path) } +// Getenv is an os.Getenv wrapper. func Getenv(name string) string { log.Printf("[DEBUG] reading the %s environmental variable", name) return os.Getenv(name) } +// LookupEnv is an os.LookupEnv wrapper. func LookupEnv(name string) (string, bool) { log.Printf("[DEBUG] reading the %s environmental variable", name) return os.LookupEnv(name) @@ -29,7 +38,7 @@ func LookupEnv(name string) (string, bool) { // ReadFileSafe checks if a file is safe to read, and then reads it. func ReadFileSafe(path string) ([]byte, error) { - if err := validateFile(path); err != nil { + if err := fileIsSafeToRead(path); err != nil { return nil, err } return readFile(path) @@ -40,7 +49,7 @@ func readFile(path string) ([]byte, error) { return os.ReadFile(path) } -func validateFile(path string) error { +func fileIsSafeToRead(path string) error { fileinfo, err := Stat(path) if err != nil { return fmt.Errorf("reading information about the config file: %w", err) @@ -51,6 +60,7 @@ func validateFile(path string) error { return nil } +// UserHomeDir is an os.UserHomeDir wrapper. func UserHomeDir() (string, error) { log.Printf("[DEBUG] reading the user home directory location from the operating system") return os.UserHomeDir() diff --git a/pkg/internal/os/os_test.go b/pkg/internal/oswrapper/os_test.go similarity index 82% rename from pkg/internal/os/os_test.go rename to pkg/internal/oswrapper/os_test.go index 44cac2d0e88..f108246e46d 100644 --- a/pkg/internal/os/os_test.go +++ b/pkg/internal/oswrapper/os_test.go @@ -1,27 +1,27 @@ -package os_test +package oswrapper_test import ( "fmt" "testing" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/os" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/oswrapper" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/testhelpers" "github.com/stretchr/testify/require" ) func TestLoadConfigFileThatIsTooBig(t *testing.T) { - if os.IsRunningOnWindows { + if oswrapper.IsRunningOnWindows() { t.Skip("checking file sizes on Windows is currently done in manual tests package") } c := make([]byte, 11*1024*1024) configPath := testhelpers.TestFile(t, "config", c) - _, err := os.ReadFileSafe(configPath) + _, err := oswrapper.ReadFileSafe(configPath) require.ErrorContains(t, err, fmt.Sprintf("config file %s is too big - maximum allowed size is 10MB", configPath)) } func TestLoadConfigFileThatDoesNotExist(t *testing.T) { configPath := "non-existing" - _, err := os.ReadFileSafe(configPath) + _, err := oswrapper.ReadFileSafe(configPath) require.ErrorContains(t, err, fmt.Sprintf("reading information about the config file: stat %s: no such file or directory", configPath)) } diff --git a/pkg/manual_tests/windows/files_test.go b/pkg/manual_tests/windows/files_test.go index f10b880b687..176aad48c46 100644 --- a/pkg/manual_tests/windows/files_test.go +++ b/pkg/manual_tests/windows/files_test.go @@ -4,14 +4,14 @@ import ( "fmt" "testing" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/os" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/oswrapper" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/testhelpers" "github.com/stretchr/testify/require" ) func TestLoadConfigFileThatIsTooBig(t *testing.T) { - if !os.IsRunningOnWindows { + if !oswrapper.IsRunningOnWindows() { t.Skip("checking file sizes on other platforms is currently done in the sdk package") } c := make([]byte, 11*1024*1024) diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 3351d5071c6..e2bfe666fa4 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -10,7 +10,7 @@ import ( "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testenvs" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/datasources" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/os" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/oswrapper" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider/docs" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider/validators" @@ -468,7 +468,7 @@ func getResources() map[string]*schema.Resource { "snowflake_warehouse": resources.Warehouse(), } - if os.Getenv(string(testenvs.EnableObjectRenamingTest)) != "" { + if oswrapper.Getenv(string(testenvs.EnableObjectRenamingTest)) != "" { resourceList["snowflake_object_renaming"] = resources.ObjectRenamingListsAndSets() } @@ -532,7 +532,7 @@ var ( func ConfigureProvider(ctx context.Context, s *schema.ResourceData) (any, diag.Diagnostics) { // hacky way to speed up our acceptance tests - if os.Getenv("TF_ACC") != "" && os.Getenv("SF_TF_ACC_TEST_CONFIGURE_CLIENT_ONCE") == "true" { + if oswrapper.Getenv("TF_ACC") != "" && oswrapper.Getenv("SF_TF_ACC_TEST_CONFIGURE_CLIENT_ONCE") == "true" { if configureProviderCtx != nil { return configureProviderCtx, nil } @@ -562,12 +562,12 @@ func ConfigureProvider(ctx context.Context, s *schema.ResourceData) (any, diag.D providerCtx.EnabledFeatures = expandStringList(v.(*schema.Set).List()) } - if os.Getenv("TF_ACC") != "" && os.Getenv("SF_TF_ACC_TEST_ENABLE_ALL_PREVIEW_FEATURES") == "true" { + if oswrapper.Getenv("TF_ACC") != "" && oswrapper.Getenv("SF_TF_ACC_TEST_ENABLE_ALL_PREVIEW_FEATURES") == "true" { providerCtx.EnabledFeatures = previewfeatures.AllPreviewFeatures } // needed for tests verifying different provider setups - if os.Getenv(resource.EnvTfAcc) != "" && os.Getenv(string(testenvs.ConfigureClientOnce)) == "true" { + if oswrapper.Getenv(resource.EnvTfAcc) != "" && oswrapper.Getenv(string(testenvs.ConfigureClientOnce)) == "true" { configureProviderCtx = providerCtx configureClientError = clientErr } else { diff --git a/pkg/provider/provider_acceptance_test.go b/pkg/provider/provider_acceptance_test.go index c30aee9e55c..e01f23ae366 100644 --- a/pkg/provider/provider_acceptance_test.go +++ b/pkg/provider/provider_acceptance_test.go @@ -301,8 +301,10 @@ func TestAcc_Provider_tomlConfigIsTooBig(t *testing.T) { acc.TestAccPreCheck(t) t.Setenv(string(testenvs.ConfigureClientOnce), "") - tmpServiceUser := acc.TestClient().SetUpTemporaryServiceUser(t) - tmpServiceUserConfig := acc.TestClient().TempTooBigTomlConfigForServiceUser(t, tmpServiceUser) + c := make([]byte, 11*1024*1024) + tomlConfig := acc.TestClient().StoreTempTomlConfig(t, func(profile string) string { + return string(c) + }) resource.Test(t, resource.TestCase{ ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, @@ -312,10 +314,10 @@ func TestAcc_Provider_tomlConfigIsTooBig(t *testing.T) { Steps: []resource.TestStep{ { PreConfig: func() { - t.Setenv(snowflakeenvs.ConfigPath, tmpServiceUserConfig.Path) + t.Setenv(snowflakeenvs.ConfigPath, tomlConfig.Path) }, - Config: config.FromModels(t, providermodel.SnowflakeProvider().WithProfile(tmpServiceUserConfig.Profile), datasourceModel()), - ExpectError: regexp.MustCompile(fmt.Sprintf("could not load config file: config file %s is too big - maximum allowed size is 10MB", tmpServiceUserConfig.Path)), + Config: config.FromModels(t, providermodel.SnowflakeProvider().WithProfile(tomlConfig.Path), datasourceModel()), + ExpectError: regexp.MustCompile(fmt.Sprintf("could not load config file: config file %s is too big - maximum allowed size is 10MB", tomlConfig.Path)), }, }, }) diff --git a/pkg/sdk/config.go b/pkg/sdk/config.go index cf1fc628e77..40fcb835396 100644 --- a/pkg/sdk/config.go +++ b/pkg/sdk/config.go @@ -14,7 +14,7 @@ import ( "time" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/collections" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/os" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/oswrapper" "github.com/pelletier/go-toml/v2" "github.com/snowflakedb/gosnowflake" "github.com/youmark/pkcs8" @@ -199,12 +199,12 @@ func boolToConfigBool(v bool) gosnowflake.ConfigBool { func GetConfigFileName() (string, error) { // has the user overridden the default config path? - if configPath, ok := os.LookupEnv("SNOWFLAKE_CONFIG_PATH"); ok { + if configPath, ok := oswrapper.LookupEnv("SNOWFLAKE_CONFIG_PATH"); ok { if configPath != "" { return configPath, nil } } - dir, err := os.UserHomeDir() + dir, err := oswrapper.UserHomeDir() if err != nil { return "", err } @@ -361,7 +361,7 @@ func pointerUrlAttributeSet(src *string, dst **url.URL) error { } func LoadConfigFile(path string) (map[string]ConfigDTO, error) { - dat, err := os.ReadFileSafe(path) + dat, err := oswrapper.ReadFileSafe(path) if err != nil { return nil, err }