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

…of provider schema (#9050) (#6358) * 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 Signed-off-by: Modular Magician <[email protected]>
hashicorp · Sep 23, 2023 · 5eb5f99 · 5eb5f99
1 parent dd58dbd
commit 5eb5f99
Show file tree

Hide file tree

Showing 7 changed files with 256 additions and 38 deletions.
diff --git a/.changelog/9050.txt b/.changelog/9050.txt
@@ -0,0 +1,3 @@
+```release-note:breaking-change
+provider: added provider-level validation so these fields are not set as empty strings in a user's config: `credentials`, `access_token`, `impersonate_service_account`, `project`, `billing_project`, `region`, `zone`
+```
diff --git a/google-beta/fwprovider/framework_provider.go b/google-beta/fwprovider/framework_provider.go
@@ -70,6 +70,7 @@ func (p *FrameworkProvider) Schema(_ context.Context, _ provider.SchemaRequest,
 						path.MatchRoot("access_token"),
 					}...),
 					CredentialsValidator(),
+					NonEmptyStringValidator(),
 				},
 			},
 			"access_token": schema.StringAttribute{
@@ -78,26 +79,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,

diff --git a/google-beta/fwprovider/framework_validators.go b/google-beta/fwprovider/framework_validators.go
@@ -86,3 +86,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{}
+}
diff --git a/google-beta/provider/provider.go b/google-beta/provider/provider.go
@@ -135,8 +135,6 @@ import (
 	"github.com/hashicorp/terraform-provider-google-beta/google-beta/tpgiamresource"
 	transport_tpg "github.com/hashicorp/terraform-provider-google-beta/google-beta/transport"
 	"github.com/hashicorp/terraform-provider-google-beta/google-beta/verify"
-
-	googleoauth "golang.org/x/oauth2/google"
 )
 
 // Provider returns a *schema.Provider.
@@ -165,12 +163,14 @@ func Provider() *schema.Provider {
 			"access_token": {
 				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": {
@@ -180,23 +180,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": {
@@ -2138,25 +2142,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{}

diff --git a/google-beta/provider/provider_internal_test.go b/google-beta/provider/provider_internal_test.go
@@ -44,12 +44,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,
@@ -71,15 +72,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])
 				}
 			}
 		})

diff --git a/google-beta/provider/provider_test.go b/google-beta/provider/provider_test.go
@@ -295,6 +295,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" {
@@ -542,3 +611,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)
+}