From cc6819730d78e6e8fb088b107bf0b651ee919d57 Mon Sep 17 00:00:00 2001 From: Modular Magician Date: Tue, 11 Apr 2023 19:39:10 +0000 Subject: [PATCH] Fix empty string credentials validation issue, increase test coverage of credential validation (#7690) Signed-off-by: Modular Magician --- .changelog/7690.txt | 3 + google/framework_provider_test.go | 101 ++++++++++++++++++------------ google/framework_validators.go | 3 +- google/provider_test.go | 84 +++++++++++++++++++------ 4 files changed, 131 insertions(+), 60 deletions(-) create mode 100644 .changelog/7690.txt diff --git a/.changelog/7690.txt b/.changelog/7690.txt new file mode 100644 index 00000000000..c45f1513f9b --- /dev/null +++ b/.changelog/7690.txt @@ -0,0 +1,3 @@ +```release-note:bug +provider: fixed bug where `credentials` field could not be set as an empty string +``` diff --git a/google/framework_provider_test.go b/google/framework_provider_test.go index 2efed8762a9..a0650e92d22 100644 --- a/google/framework_provider_test.go +++ b/google/framework_provider_test.go @@ -57,48 +57,69 @@ func TestFrameworkProvider_impl(t *testing.T) { var _ provider.ProviderWithMetaSchema = New("test") } -func TestFrameworkProvider_loadCredentialsFromFile(t *testing.T) { - cv := CredentialsValidator() - - req := validator.StringRequest{ - ConfigValue: types.StringValue(testFakeCredentialsPath), - } - - resp := validator.StringResponse{ - Diagnostics: diag.Diagnostics{}, - } - - cv.ValidateString(context.Background(), req, &resp) - - if resp.Diagnostics.WarningsCount() > 0 { - t.Errorf("Expected 0 warnings, got %d", resp.Diagnostics.WarningsCount()) - } - if resp.Diagnostics.HasError() { - t.Errorf("Expected 0 errors, got %d", resp.Diagnostics.ErrorsCount()) - } -} - -func TestFrameworkProvider_loadCredentialsFromJSON(t *testing.T) { - contents, err := ioutil.ReadFile(testFakeCredentialsPath) - if err != nil { - t.Fatalf("Unexpected error: %s", err) - } - cv := CredentialsValidator() - - req := validator.StringRequest{ - ConfigValue: types.StringValue(string(contents)), - } - - resp := validator.StringResponse{ - Diagnostics: diag.Diagnostics{}, +func TestFrameworkProvider_CredentialsValidator(t *testing.T) { + cases := map[string]struct { + ConfigValue func(t *testing.T) types.String + ExpectedWarningCount int + ExpectedErrorCount int + }{ + "configuring credentials as a path to a credentials JSON file is valid": { + ConfigValue: func(t *testing.T) types.String { + return types.StringValue(testFakeCredentialsPath) // Path to a test fixture + }, + }, + "configuring credentials as a path to a non-existant file is NOT valid": { + ConfigValue: func(t *testing.T) types.String { + return types.StringValue("./this/path/doesnt/exist.json") // Doesn't exist + }, + ExpectedErrorCount: 1, + }, + "configuring credentials as a credentials JSON string is valid": { + ConfigValue: func(t *testing.T) types.String { + contents, err := ioutil.ReadFile(testFakeCredentialsPath) + if err != nil { + t.Fatalf("Unexpected error: %s", err) + } + stringContents := string(contents) + return types.StringValue(stringContents) + }, + }, + "configuring credentials as an empty string is valid": { + ConfigValue: func(t *testing.T) types.String { + return types.StringValue("") + }, + }, + "leaving credentials unconfigured is valid": { + ConfigValue: func(t *testing.T) types.String { + return types.StringNull() + }, + }, } - cv.ValidateString(context.Background(), req, &resp) - if resp.Diagnostics.WarningsCount() > 0 { - t.Errorf("Expected 0 warnings, got %d", resp.Diagnostics.WarningsCount()) - } - if resp.Diagnostics.HasError() { - t.Errorf("Expected 0 errors, got %d", resp.Diagnostics.ErrorsCount()) + for tn, tc := range cases { + t.Run(tn, func(t *testing.T) { + // Arrange + req := validator.StringRequest{ + ConfigValue: tc.ConfigValue(t), + } + + resp := validator.StringResponse{ + Diagnostics: diag.Diagnostics{}, + } + + cv := CredentialsValidator() + + // Act + cv.ValidateString(context.Background(), req, &resp) + + // Assert + if resp.Diagnostics.WarningsCount() > tc.ExpectedWarningCount { + t.Errorf("Expected %d warnings, got %d", tc.ExpectedWarningCount, resp.Diagnostics.WarningsCount()) + } + if resp.Diagnostics.ErrorsCount() > tc.ExpectedErrorCount { + t.Errorf("Expected %d errors, got %d", tc.ExpectedErrorCount, resp.Diagnostics.ErrorsCount()) + } + }) } } diff --git a/google/framework_validators.go b/google/framework_validators.go index 2391b1a221f..f570e85bec0 100644 --- a/google/framework_validators.go +++ b/google/framework_validators.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework-validators/stringvalidator" "github.com/hashicorp/terraform-plugin-framework/schema/validator" + "github.com/hashicorp/terraform-plugin-framework/types" googleoauth "golang.org/x/oauth2/google" ) @@ -36,7 +37,7 @@ func (v credentialsValidator) MarkdownDescription(ctx context.Context) string { // ValidateString performs the validation. func (v credentialsValidator) ValidateString(ctx context.Context, request validator.StringRequest, response *validator.StringResponse) { - if request.ConfigValue.IsNull() || request.ConfigValue.IsUnknown() { + if request.ConfigValue.IsNull() || request.ConfigValue.IsUnknown() || request.ConfigValue.Equal(types.StringValue("")) { return } diff --git a/google/provider_test.go b/google/provider_test.go index 4ac6ee3b6ca..26644aad21a 100644 --- a/google/provider_test.go +++ b/google/provider_test.go @@ -2,6 +2,7 @@ package google import ( "context" + "errors" "fmt" "io/ioutil" "log" @@ -165,27 +166,72 @@ func AccTestPreCheck(t *testing.T) { } } -func TestProvider_loadCredentialsFromFile(t *testing.T) { - ws, es := validateCredentials(testFakeCredentialsPath, "") - if len(ws) != 0 { - t.Errorf("Expected %d warnings, got %v", len(ws), ws) - } - if len(es) != 0 { - t.Errorf("Expected %d errors, got %v", len(es), es) +func TestProvider_validateCredentials(t *testing.T) { + cases := map[string]struct { + ConfigValue func(t *testing.T) interface{} + ValueNotProvided bool + ExpectedWarnings []string + ExpectedErrors []error + }{ + "configuring credentials as a path to a credentials JSON file is valid": { + ConfigValue: func(t *testing.T) interface{} { + return testFakeCredentialsPath // Path to a test fixture + }, + }, + "configuring credentials as a path to a non-existant file is NOT valid": { + ConfigValue: func(t *testing.T) interface{} { + return "./this/path/doesnt/exist.json" // Doesn't exist + }, + ExpectedErrors: []error{ + // As the file doesn't exist, so the function attempts to parse it as a JSON + errors.New("JSON credentials are not valid: invalid character '.' looking for beginning of value"), + }, + }, + "configuring credentials as a credentials JSON string is valid": { + ConfigValue: func(t *testing.T) interface{} { + contents, err := ioutil.ReadFile(testFakeCredentialsPath) + if err != nil { + t.Fatalf("Unexpected error: %s", err) + } + return string(contents) + }, + }, + "configuring credentials as an empty string is valid": { + ConfigValue: func(t *testing.T) interface{} { + return "" + }, + }, + "leaving credentials unconfigured is valid": { + ValueNotProvided: true, + }, } -} -func TestProvider_loadCredentialsFromJSON(t *testing.T) { - contents, err := ioutil.ReadFile(testFakeCredentialsPath) - if err != nil { - t.Fatalf("Unexpected error: %s", err) - } - ws, es := validateCredentials(string(contents), "") - if len(ws) != 0 { - t.Errorf("Expected %d warnings, got %v", len(ws), ws) - } - if len(es) != 0 { - t.Errorf("Expected %d errors, got %v", len(es), es) + for tn, tc := range cases { + t.Run(tn, func(t *testing.T) { + // Arrange + var configValue interface{} + if !tc.ValueNotProvided { + configValue = tc.ConfigValue(t) + } + + // Act + // Note: second argument is currently unused by the function but is necessary to fulfill the SchemaValidateFunc type's function signature + ws, es := validateCredentials(configValue, "") + + // Assert + if len(ws) != len(tc.ExpectedWarnings) { + t.Errorf("Expected %d warnings, got %d: %v", len(tc.ExpectedWarnings), len(ws), ws) + } + if len(es) != len(tc.ExpectedErrors) { + t.Errorf("Expected %d errors, got %d: %v", len(tc.ExpectedErrors), len(es), es) + } + + if len(tc.ExpectedErrors) > 0 { + if es[0].Error() != tc.ExpectedErrors[0].Error() { + t.Errorf("Expected first error to be \"%s\", got \"%s\"", tc.ExpectedErrors[0], es[0]) + } + } + }) } }