backendbase: Preserve unset arguments as null when not set by env

Previously we tried a simplification where any attribute with an "SDK-like
default" was guaranteed to never be null, because that seemed like a
plausible mimic of the legacy SDK's general aversion to nulls.

However, the SDK does still respect the absense of an argument as different
to its zero value in one narrow case: when the argument isn't set in the
configuration, also isn't set by any environment variables, and has no
static fallback default. In that case, the SDK's built-in validation rules
such as "ConflictsWith" _do_ treat absent as different from zero-value.

To allow backends to continue making such distinctions themselves where
that's useful, SDKLikeDefaults now leaves an attribute set to null if it
wasn't present in the configuration and none of the environment variables
or fallback value cause it to end up being a non-empty string. The backend
can then choose to bypass the SDKLikeData API and check the cty.Value
directly if it wants to distinguish set from zero even though SDKLikeData
is designed to avoid the need to do that.

This also updates the gcs backend to make use of the new facility, since
it wants to raise an error if two arguments are both set even if they are
both set to the empty string. Restoring that original error case is the
main motivation for this change.

internal/backend/backendbase/base.go

@@ -38,9 +38,11 @@ type Base struct {
 	// primitive-typed toplevel attribute in Schema, and PrepareConfig will
 	// arrange for the default values to be inserted before it returns.
 	//
-	// In particular, note that any attribute with an entry in this definition
-	// is guaranteed to never be null, since PrepareConfig will replace any
-	// nulls with an SDK-like "zero value".
+	// As a special case, if the value in the configuration is unset (null),
+	// none of the environment variables are non-empty, and the fallback
+	// value is empty, then the attribute value will be left as null in the
+	// object returned by PrepareConfig. In all other situations an attribute
+	// specified here is definitely not null.
 	SDKLikeDefaults SDKLikeDefaults
 }
 

internal/backend/backendbase/sdklike.go

@@ -227,6 +227,18 @@ func (d SDKLikeDefaults) ApplyTo(base cty.Value) (cty.Value, error) {
 			return cty.NilVal, fmt.Errorf("argument %q is required", attrName)
 		}
 
+		// As a special case, if we still have an empty string and the original
+		// value was null then we'll preserve the null. This is a compromise,
+		// assuming that SDKLikeData knows how to treat a null value as a
+		// zero value anyway and if we preserve the null then the recipient
+		// of this result can still use the cty.Value result directly to
+		// distinguish between the value being set explicitly to empty in
+		// the config vs. being entirely unset.
+		if rawStr == "" && givenVal.IsNull() {
+			retAttrs[attrName] = givenVal
+			continue
+		}
+
 		// By the time we get here, rawStr should be empty only if the original
 		// value was unset and all of the fallback environment variables were
 		// also unset. Otherwise, rawStr contains a string representation of

internal/backend/backendbase/sdklike_test.go

@@ -269,7 +269,7 @@ func TestSDKLikeApplyEnvDefaults(t *testing.T) {
 			"string_env_empty":       cty.StringVal("beep from environment"),
 			"string_env_unsetfirst":  cty.StringVal("beep from environment"),
 			"string_env_unsetsecond": cty.StringVal("beep from environment"),
-			"string_nothing_null":    cty.StringVal(""),
+			"string_nothing_null":    cty.NullVal(cty.String),
 			"string_nothing_empty":   cty.StringVal(""),
 			"passthru":               cty.EmptyObjectVal,
 		})

internal/backend/remote-state/gcs/backend.go

@@ -148,6 +148,19 @@ func (b *Backend) Configure(configVal cty.Value) tfdiags.Diagnostics {
 			fmt.Errorf("can't set both encryption_key and kms_encryption_key"),
 		)
 	}
+	// The above catches the main case where both of the arguments are set to
+	// a non-empty value, but we also want to reject the situation where
+	// both are present in the configuration regardless of what values were
+	// assigned to them. (This check doesn't take the environment variables
+	// into account, so must allow neither to be set in the main configuration.)
+	if !(configVal.GetAttr("encryption_key").IsNull() || configVal.GetAttr("kms_encryption_key").IsNull()) {
+		// This rejects a configuration like:
+		//     encryption_key     = ""
+		//     kms_encryption_key = ""
+		return backendbase.ErrorAsDiagnostics(
+			fmt.Errorf("can't set both encryption_key and kms_encryption_key"),
+		)
+	}
 
 	b.bucketName = data.String("bucket")
 	b.prefix = strings.TrimLeft(data.String("prefix"), "/")

internal/backend/remote-state/gcs/backend_test.go

@@ -18,6 +18,7 @@ import (
 	"github.com/hashicorp/terraform/internal/backend"
 	"github.com/hashicorp/terraform/internal/httpclient"
 	"github.com/hashicorp/terraform/internal/states/remote"
+	"github.com/zclconf/go-cty/cty"
 	"google.golang.org/api/option"
 	kmspb "google.golang.org/genproto/googleapis/cloud/kms/v1"
 )
@@ -211,6 +212,56 @@ func TestBackendWithCustomerManagedKMSEncryption(t *testing.T) {
 	backend.TestBackendStateLocks(t, be0, be1)
 }
 
+func TestBackendEncryptionKeyEmptyConflict(t *testing.T) {
+	// This test is for the edge case where encryption_key and
+	// kms_encryption_key are both set in the configuration but set to empty
+	// strings. The "SDK-like" helpers treat unset as empty string, so
+	// we need an extra rule to catch them both being set to empty string
+	// directly inside the configuration, and this test covers that
+	// special case.
+	//
+	// The following assumes that the validation check we're testing will, if
+	// failing, always block attempts to reach any real GCP services, and so
+	// this test should be fine to run without an acceptance testing opt-in.
+
+	// This test is for situations where these environment variables are not set.
+	t.Setenv("GOOGLE_ENCRYPTION_KEY", "")
+	t.Setenv("GOOGLE_KMS_ENCRYPTION_KEY", "")
+
+	backend := New()
+	schema := backend.ConfigSchema()
+	rawVal := cty.ObjectVal(map[string]cty.Value{
+		"bucket": cty.StringVal("fake-placeholder"),
+
+		// These are both empty strings but should still be considered as
+		// set when we enforce teh rule that they can't both be set at once.
+		"encryption_key":     cty.StringVal(""),
+		"kms_encryption_key": cty.StringVal(""),
+	})
+	// The following mimicks how the terraform_remote_state data source
+	// treats its "config" argument, which is a realistic situation where
+	// we take an arbitrary object and try to force it to conform to the
+	// backend's schema.
+	configVal, err := schema.CoerceValue(rawVal)
+	if err != nil {
+		t.Fatalf("unexpected coersion error: %s", err)
+	}
+	configVal, diags := backend.PrepareConfig(configVal)
+	if diags.HasErrors() {
+		t.Fatalf("unexpected PrepareConfig error: %s", diags.Err().Error())
+	}
+
+	configDiags := backend.Configure(configVal)
+	if !configDiags.HasErrors() {
+		t.Fatalf("unexpected success; want error")
+	}
+	gotErr := configDiags.Err().Error()
+	wantErr := `can't set both encryption_key and kms_encryption_key`
+	if !strings.Contains(gotErr, wantErr) {
+		t.Errorf("wrong error\ngot: %s\nwant substring: %s", gotErr, wantErr)
+	}
+}
+
 // setupBackend returns a new GCS backend.
 func setupBackend(t *testing.T, bucket, prefix, key, kmsName string) backend.Backend {
 	t.Helper()