diff --git a/internal/backend/backendbase/base.go b/internal/backend/backendbase/base.go index 54d15d8b1911..98890d2a179f 100644 --- a/internal/backend/backendbase/base.go +++ b/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 } diff --git a/internal/backend/backendbase/sdklike.go b/internal/backend/backendbase/sdklike.go index 74c8afef6112..9d53cbf6b543 100644 --- a/internal/backend/backendbase/sdklike.go +++ b/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 diff --git a/internal/backend/backendbase/sdklike_test.go b/internal/backend/backendbase/sdklike_test.go index 618a69ce2a39..bca3b8a4ebf8 100644 --- a/internal/backend/backendbase/sdklike_test.go +++ b/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, }) diff --git a/internal/backend/remote-state/gcs/backend.go b/internal/backend/remote-state/gcs/backend.go index f22b80805fab..fb2c24a90dac 100644 --- a/internal/backend/remote-state/gcs/backend.go +++ b/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"), "/") diff --git a/internal/backend/remote-state/gcs/backend_test.go b/internal/backend/remote-state/gcs/backend_test.go index bcf2c0f591cb..cb240e584880 100644 --- a/internal/backend/remote-state/gcs/backend_test.go +++ b/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()