From 660ff86c4cccf5528f23adcd9e2a1936ea4af67a Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 19 Apr 2024 10:36:36 -0700 Subject: [PATCH] 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 | 8 +-- internal/backend/backendbase/sdklike.go | 12 +++++ internal/backend/backendbase/sdklike_test.go | 2 +- internal/backend/remote-state/gcs/backend.go | 13 +++++ .../backend/remote-state/gcs/backend_test.go | 51 +++++++++++++++++++ 5 files changed, 82 insertions(+), 4 deletions(-) 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()