Add validation to provider fields in both SDK and PF implementations …

…of provider schema (#9050)

* Add empty-string validator for PF provider

* Add empty-string validator for SDK provider, move SDK validators to separate file

* Add empty string validators to : credentials, access_token, impersonate_service_account, project, billing_project, region, zone

* Add unit tests for `ValidateEmptyStrings`

* Remove empty string test case from `ValidateCredentials`

* Add acceptace tests showing that empty strings in provider block results in a validation error, and empty provider blocks have no validation errors

* Make the SDK provider's `ValidateCredentials` validator reject empty strings

* Update acceptance test after change in `credentials` validation

* Fix test definitions to avoid fall-through

* Update validation error message in code and tests

mmv1/third_party/terraform/fwprovider/framework_provider.go.erb

@@ -71,6 +71,7 @@ func (p *FrameworkProvider) Schema(_ context.Context, _ provider.SchemaRequest,
                         path.MatchRoot("access_token"),
                     }...),
                     CredentialsValidator(),
+                    NonEmptyStringValidator(),
                 },
             },
             "access_token": schema.StringAttribute{
@@ -79,26 +80,42 @@ func (p *FrameworkProvider) Schema(_ context.Context, _ provider.SchemaRequest,
                     stringvalidator.ConflictsWith(path.Expressions{
                         path.MatchRoot("credentials"),
                     }...),
+                    NonEmptyStringValidator(),
                 },
             },
             "impersonate_service_account": schema.StringAttribute{
                 Optional: true,
+                Validators: []validator.String{
+                    NonEmptyStringValidator(),
+                },
             },
             "impersonate_service_account_delegates": schema.ListAttribute{
                 Optional:    true,
                 ElementType: types.StringType,
             },
             "project": schema.StringAttribute{
                 Optional: true,
+                Validators: []validator.String{
+                    NonEmptyStringValidator(),
+                },
             },
             "billing_project": schema.StringAttribute{
                 Optional: true,
+                Validators: []validator.String{
+                    NonEmptyStringValidator(),
+                },
             },
             "region": schema.StringAttribute{
                 Optional: true,
+                Validators: []validator.String{
+                    NonEmptyStringValidator(),
+                },
             },
             "zone": schema.StringAttribute{
                 Optional: true,
+                Validators: []validator.String{
+                    NonEmptyStringValidator(),
+                },
             },
             "scopes": schema.ListAttribute{
                 Optional:    true,

mmv1/third_party/terraform/fwprovider/framework_validators.go

@@ -84,3 +84,34 @@ func (v nonnegativedurationValidator) ValidateString(ctx context.Context, reques
 func NonNegativeDurationValidator() validator.String {
 	return nonnegativedurationValidator{}
 }
+
+// Non Empty String Validator
+type nonEmptyStringValidator struct {
+}
+
+// Description describes the validation in plain text formatting.
+func (v nonEmptyStringValidator) Description(_ context.Context) string {
+	return "value expected to be a string that isn't an empty string"
+}
+
+// MarkdownDescription describes the validation in Markdown formatting.
+func (v nonEmptyStringValidator) MarkdownDescription(ctx context.Context) string {
+	return v.Description(ctx)
+}
+
+// ValidateString performs the validation.
+func (v nonEmptyStringValidator) ValidateString(ctx context.Context, request validator.StringRequest, response *validator.StringResponse) {
+	if request.ConfigValue.IsNull() || request.ConfigValue.IsUnknown() {
+		return
+	}
+
+	value := request.ConfigValue.ValueString()
+
+	if value == "" {
+		response.Diagnostics.AddError("expected a non-empty string", fmt.Sprintf("%s was set to `%s`", request.Path, value))
+	}
+}
+
+func NonEmptyStringValidator() validator.String {
+	return nonEmptyStringValidator{}
+}

mmv1/third_party/terraform/provider/provider.go.erb

@@ -39,8 +39,6 @@ import (
 	"github.com/hashicorp/terraform-provider-google/google/tpgiamresource"
 	transport_tpg "github.com/hashicorp/terraform-provider-google/google/transport"
 	"github.com/hashicorp/terraform-provider-google/google/verify"
-
-	googleoauth "golang.org/x/oauth2/google"
 )
 
 // Provider returns a *schema.Provider.
@@ -60,21 +58,23 @@ func Provider() *schema.Provider {
 	provider := &schema.Provider{
 		Schema: map[string]*schema.Schema{
 			"credentials": {
-				Type:     schema.TypeString,
-				Optional: true,
-				ValidateFunc: ValidateCredentials,
+				Type:          schema.TypeString,
+				Optional:      true,
+				ValidateFunc:  ValidateCredentials,
 				ConflictsWith: []string{"access_token"},
 			},
 
 			"access_token": {
-				Type:     schema.TypeString,
-				Optional: true,
+				Type:          schema.TypeString,
+				Optional:      true,
+				ValidateFunc:  ValidateEmptyStrings,
 				ConflictsWith: []string{"credentials"},
 			},
 
 			"impersonate_service_account": {
-				Type:     schema.TypeString,
-				Optional: true,
+				Type:         schema.TypeString,
+				Optional:     true,
+				ValidateFunc: ValidateEmptyStrings,
 			},
 
 			"impersonate_service_account_delegates": {
@@ -84,23 +84,27 @@ func Provider() *schema.Provider {
 			},
 
 			"project": {
-				Type:     schema.TypeString,
-				Optional: true,
+				Type:         schema.TypeString,
+				Optional:     true,
+				ValidateFunc: ValidateEmptyStrings,
 			},
 
 			"billing_project": {
-				Type:     schema.TypeString,
-				Optional: true,
+				Type:         schema.TypeString,
+				Optional:     true,
+				ValidateFunc: ValidateEmptyStrings,
 			},
 
 			"region": {
-				Type:     schema.TypeString,
-				Optional: true,
+				Type:         schema.TypeString,
+				Optional:     true,
+				ValidateFunc: ValidateEmptyStrings,
 			},
 
 			"zone": {
-				Type:     schema.TypeString,
-				Optional: true,
+				Type:         schema.TypeString,
+				Optional:     true,
+				ValidateFunc: ValidateEmptyStrings,
 			},
 
 			"scopes": {
@@ -771,25 +775,6 @@ func ProviderConfigure(ctx context.Context, d *schema.ResourceData, p *schema.Pr
 	return transport_tpg.ProviderDCLConfigure(d, &config), nil
 }
 
-func ValidateCredentials(v interface{}, k string) (warnings []string, errors []error) {
-	if v == nil || v.(string) == "" {
-		return
-	}
-	// NOTE: Above we have to allow empty string as valid because we don't know if it's a zero value or not
-
-	creds := v.(string)
-	// if this is a path and we can stat it, assume it's ok
-	if _, err := os.Stat(creds); err == nil {
-		return
-	}
-	if _, err := googleoauth.CredentialsFromJSON(context.Background(), []byte(creds)); err != nil {
-		errors = append(errors,
-			fmt.Errorf("JSON credentials are not valid: %s", err))
-	}
-
-	return
-}
-
 func mergeResourceMaps(ms ...map[string]*schema.Resource) (map[string]*schema.Resource, error) {
 	merged := make(map[string]*schema.Resource)
 	duplicates := []string{}

mmv1/third_party/terraform/provider/provider_internal_test.go

@@ -42,12 +42,13 @@ func TestProvider_ValidateCredentials(t *testing.T) {
 				return string(contents)
 			},
 		},
-		// There's a risk of changing the validator to saying "" is invalid, as it may mean that
-		// everyone not using the credentials field would get validation errors.
-		"configuring credentials as an empty string is not identified as invalid by the function, as it can't distinguish from zero values ": {
+		"configuring credentials as an empty string is not valid": {
 			ConfigValue: func(t *testing.T) interface{} {
 				return ""
 			},
+			ExpectedErrors: []error{
+				errors.New("expected a non-empty string"),
+			},
 		},
 		"leaving credentials unconfigured is valid": {
 			ValueNotProvided: true,
@@ -69,15 +70,65 @@ func TestProvider_ValidateCredentials(t *testing.T) {
 
 			// Assert
 			if len(ws) != len(tc.ExpectedWarnings) {
-				t.Errorf("Expected %d warnings, got %d: %v", len(tc.ExpectedWarnings), len(ws), ws)
+				t.Fatalf("Expected %d warnings, got %d: %v", len(tc.ExpectedWarnings), len(ws), ws)
+			}
+			if len(es) != len(tc.ExpectedErrors) {
+				t.Fatalf("Expected %d errors, got %d: %v", len(tc.ExpectedErrors), len(es), es)
+			}
+
+			if len(tc.ExpectedErrors) > 0 && len(es) > 0 {
+				if es[0].Error() != tc.ExpectedErrors[0].Error() {
+					t.Fatalf("Expected first error to be \"%s\", got \"%s\"", tc.ExpectedErrors[0], es[0])
+				}
+			}
+		})
+	}
+}
+
+func TestProvider_ValidateEmptyStrings(t *testing.T) {
+	cases := map[string]struct {
+		ConfigValue      interface{}
+		ValueNotProvided bool
+		ExpectedWarnings []string
+		ExpectedErrors   []error
+	}{
+		"non-empty strings are valid": {
+			ConfigValue: "foobar",
+		},
+		"unconfigured values are valid": {
+			ValueNotProvided: true,
+		},
+		"empty strings are not valid": {
+			ConfigValue: "",
+			ExpectedErrors: []error{
+				errors.New("expected a non-empty string"),
+			},
+		},
+	}
+	for tn, tc := range cases {
+		t.Run(tn, func(t *testing.T) {
+
+			// Arrange
+			var configValue interface{}
+			if !tc.ValueNotProvided {
+				configValue = tc.ConfigValue
+			}
+
+			// Act
+			// Note: second argument is currently unused by the function but is necessary to fulfill the SchemaValidateFunc type's function signature
+			ws, es := provider.ValidateEmptyStrings(configValue, "")
+
+			// Assert
+			if len(ws) != len(tc.ExpectedWarnings) {
+				t.Fatalf("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)
+				t.Fatalf("Expected %d errors, got %d: %v", len(tc.ExpectedErrors), len(es), es)
 			}
 
-			if len(tc.ExpectedErrors) > 0 {
+			if len(tc.ExpectedErrors) > 0 && len(es) > 0 {
 				if es[0].Error() != tc.ExpectedErrors[0].Error() {
-					t.Errorf("Expected first error to be \"%s\", got \"%s\"", tc.ExpectedErrors[0], es[0])
+					t.Fatalf("Expected first error to be \"%s\", got \"%s\"", tc.ExpectedErrors[0], es[0])
 				}
 			}
 		})

mmv1/third_party/terraform/provider/provider_test.go.erb

@@ -294,6 +294,75 @@ func TestAccProviderCredentialsUnknownValue(t *testing.T) {
 	})
 }
 
+func TestAccProviderEmptyStrings(t *testing.T) {
+	t.Parallel()
+
+	acctest.VcrTest(t, resource.TestCase{
+		PreCheck:                 func() { acctest.AccTestPreCheck(t) },
+		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t),
+		// No TestDestroy since that's not really the point of this test
+		Steps: []resource.TestStep{
+			// When no values are set in the provider block there are no errors
+			// This test case is a control to show validation doesn't accidentally flag unset fields
+			// The "" argument is a lack of key = value being passed into the provider block
+			{
+				Config:             testAccProvider_checkPlanTimeErrors("", acctest.RandString(t, 10)),
+				PlanOnly:           true,
+				ExpectNonEmptyPlan: true,
+			},
+			// credentials as an empty string causes a validation error
+			{
+				Config:             testAccProvider_checkPlanTimeErrors(`credentials = ""`, acctest.RandString(t, 10)),
+				PlanOnly:           true,
+				ExpectNonEmptyPlan: true,
+				ExpectError:        regexp.MustCompile(`expected a non-empty string`),
+			},
+			// access_token as an empty string causes a validation error
+			{
+				Config:             testAccProvider_checkPlanTimeErrors(`access_token = ""`, acctest.RandString(t, 10)),
+				PlanOnly:           true,
+				ExpectNonEmptyPlan: true,
+				ExpectError:        regexp.MustCompile(`expected a non-empty string`),
+			},
+			// impersonate_service_account as an empty string causes a validation error
+			{
+				Config:             testAccProvider_checkPlanTimeErrors(`impersonate_service_account = ""`, acctest.RandString(t, 10)),
+				PlanOnly:           true,
+				ExpectNonEmptyPlan: true,
+				ExpectError:        regexp.MustCompile(`expected a non-empty string`),
+			},
+			// project as an empty string causes a validation error
+			{
+				Config:             testAccProvider_checkPlanTimeErrors(`project = ""`, acctest.RandString(t, 10)),
+				PlanOnly:           true,
+				ExpectNonEmptyPlan: true,
+				ExpectError:        regexp.MustCompile(`expected a non-empty string`),
+			},
+			// billing_project as an empty string causes a validation error
+			{
+				Config:             testAccProvider_checkPlanTimeErrors(`billing_project = ""`, acctest.RandString(t, 10)),
+				PlanOnly:           true,
+				ExpectNonEmptyPlan: true,
+				ExpectError:        regexp.MustCompile(`expected a non-empty string`),
+			},
+			// region as an empty string causes a validation error
+			{
+				Config:             testAccProvider_checkPlanTimeErrors(`region = ""`, acctest.RandString(t, 10)),
+				PlanOnly:           true,
+				ExpectNonEmptyPlan: true,
+				ExpectError:        regexp.MustCompile(`expected a non-empty string`),
+			},
+			// zone as an empty string causes a validation error
+			{
+				Config:             testAccProvider_checkPlanTimeErrors(`zone = ""`, acctest.RandString(t, 10)),
+				PlanOnly:           true,
+				ExpectNonEmptyPlan: true,
+				ExpectError:        regexp.MustCompile(`expected a non-empty string`),
+			},
+		},
+	})
+}
+
 func testAccProviderBasePath_setBasePath(endpoint, name string) string {
 	return fmt.Sprintf(`
 provider "google" {
@@ -541,3 +610,16 @@ resource "google_firebase_project" "this" {
   ]
 }`, credentials, pid, pid, pid, org, billing)
 }
+
+func testAccProvider_checkPlanTimeErrors(providerArgument, randString string) string {
+	return fmt.Sprintf(`
+provider "google" {
+	%s
+}
+
+# A random resource so that the test can generate a plan (can't check validation errors when plan is empty)
+resource "google_pubsub_topic" "example" {
+  name = "tf-test-planned-resource-%s"
+}
+`, providerArgument, randString)
+}