From 3d211eb42ffcdc9d33d56bc2785d64b22f158c39 Mon Sep 17 00:00:00 2001 From: Liam Cervante Date: Tue, 16 Apr 2024 16:59:02 +0200 Subject: [PATCH] Fix crash when importing a resource with complex sensitive attributes (#34996) --- internal/genconfig/generate_config.go | 67 +++--------------- internal/genconfig/generate_config_test.go | 70 +++++++++++++++++++ .../terraform/context_plan_import_test.go | 66 ++++++++++++++++- 3 files changed, 143 insertions(+), 60 deletions(-) diff --git a/internal/genconfig/generate_config.go b/internal/genconfig/generate_config.go index 8acf6ed061..6f519c0ca9 100644 --- a/internal/genconfig/generate_config.go +++ b/internal/genconfig/generate_config.go @@ -38,7 +38,6 @@ func GenerateResourceContents(addr addrs.AbsResourceInstance, buf.WriteString(fmt.Sprintf("provider = %s\n", pc.StringCompact())) } - stateVal = omitUnknowns(stateVal) if stateVal.RawEquals(cty.NilVal) { diags = diags.Append(writeConfigAttributes(addr, &buf, schema.Attributes, 2)) diags = diags.Append(writeConfigBlocks(addr, &buf, schema.BlockTypes, 2)) @@ -151,11 +150,17 @@ func writeConfigAttributesFromExisting(addr addrs.AbsResourceInstance, buf *stri val = attrS.EmptyValue() } if val.Type() == cty.String { + // Before we inspect the string, take off any marks. + unmarked, marks := val.Unmark() + // SHAMELESS HACK: If we have "" for an optional value, assume // it is actually null, due to the legacy SDK. - if !val.IsNull() && attrS.Optional && len(val.AsString()) == 0 { - val = attrS.EmptyValue() + if !unmarked.IsNull() && attrS.Optional && len(unmarked.AsString()) == 0 { + unmarked = attrS.EmptyValue() } + + // Before we carry on, add the marks back. + val = unmarked.WithMarks(marks) } if attrS.Sensitive || val.IsMarked() { buf.WriteString("null # sensitive") @@ -567,59 +572,3 @@ func ctyCollectionValues(val cty.Value) []cty.Value { return ret } - -// omitUnknowns recursively walks the src cty.Value and returns a new cty.Value, -// omitting any unknowns. -// -// The result also normalizes some types: all sequence types are turned into -// tuple types and all mapping types are converted to object types, since we -// assume the result of this is just going to be serialized as JSON (and thus -// lose those distinctions) anyway. -func omitUnknowns(val cty.Value) cty.Value { - ty := val.Type() - switch { - case val.IsNull(): - return val - case !val.IsKnown(): - return cty.NilVal - case ty.IsPrimitiveType(): - return val - case ty.IsListType() || ty.IsTupleType() || ty.IsSetType(): - var vals []cty.Value - it := val.ElementIterator() - for it.Next() { - _, v := it.Element() - newVal := omitUnknowns(v) - if newVal != cty.NilVal { - vals = append(vals, newVal) - } else if newVal == cty.NilVal { - // element order is how we correlate unknownness, so we must - // replace unknowns with nulls - vals = append(vals, cty.NullVal(v.Type())) - } - } - // We use tuple types always here, because the work we did above - // may have caused the individual elements to have different types, - // and we're doing this work to produce JSON anyway and JSON marshalling - // represents all of these sequence types as an array. - return cty.TupleVal(vals) - case ty.IsMapType() || ty.IsObjectType(): - vals := make(map[string]cty.Value) - it := val.ElementIterator() - for it.Next() { - k, v := it.Element() - newVal := omitUnknowns(v) - if newVal != cty.NilVal { - vals[k.AsString()] = newVal - } - } - // We use object types always here, because the work we did above - // may have caused the individual elements to have different types, - // and we're doing this work to produce JSON anyway and JSON marshalling - // represents both of these mapping types as an object. - return cty.ObjectVal(vals) - default: - // Should never happen, since the above should cover all types - panic(fmt.Sprintf("omitUnknowns cannot handle %#v", val)) - } -} diff --git a/internal/genconfig/generate_config_test.go b/internal/genconfig/generate_config_test.go index 88829e8bf8..8f09384386 100644 --- a/internal/genconfig/generate_config_test.go +++ b/internal/genconfig/generate_config_test.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs/configschema" + "github.com/hashicorp/terraform/internal/lang/marks" ) func TestConfigGeneration(t *testing.T) { @@ -536,6 +537,67 @@ resource "tfcoremock_simple_resource" "empty" { expected: ` resource "tfcoremock_simple_resource" "empty" { value = "[\"Hello\", \"World\"" +}`, + }, + // Just try all the simple values with sensitive marks. + "sensitive_values": { + schema: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "string": sensitiveAttribute(cty.String), + "empty_string": sensitiveAttribute(cty.String), + "number": sensitiveAttribute(cty.Number), + "bool": sensitiveAttribute(cty.Bool), + "object": sensitiveAttribute(cty.Object(map[string]cty.Type{ + "nested": cty.String, + })), + "list": sensitiveAttribute(cty.List(cty.String)), + "map": sensitiveAttribute(cty.Map(cty.String)), + "set": sensitiveAttribute(cty.Set(cty.String)), + }, + }, + addr: addrs.AbsResourceInstance{ + Module: addrs.RootModuleInstance, + Resource: addrs.ResourceInstance{ + Resource: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "tfcoremock_sensitive_values", + Name: "values", + }, + Key: addrs.NoKey, + }, + }, + provider: addrs.LocalProviderConfig{ + LocalName: "tfcoremock", + }, + value: cty.ObjectVal(map[string]cty.Value{ + // Values that are sensitive will now be marked as such + "string": cty.StringVal("Hello, world!").Mark(marks.Sensitive), + "empty_string": cty.StringVal("").Mark(marks.Sensitive), + "number": cty.NumberIntVal(42).Mark(marks.Sensitive), + "bool": cty.True.Mark(marks.Sensitive), + "object": cty.ObjectVal(map[string]cty.Value{ + "nested": cty.StringVal("Hello, solar system!"), + }).Mark(marks.Sensitive), + "list": cty.ListVal([]cty.Value{ + cty.StringVal("Hello, world!"), + }).Mark(marks.Sensitive), + "map": cty.MapVal(map[string]cty.Value{ + "key": cty.StringVal("Hello, world!"), + }).Mark(marks.Sensitive), + "set": cty.SetVal([]cty.Value{ + cty.StringVal("Hello, world!"), + }).Mark(marks.Sensitive), + }), + expected: ` +resource "tfcoremock_sensitive_values" "values" { + bool = null # sensitive + empty_string = null # sensitive + list = null # sensitive + map = null # sensitive + number = null # sensitive + object = null # sensitive + set = null # sensitive + string = null # sensitive }`, }, } @@ -558,3 +620,11 @@ resource "tfcoremock_simple_resource" "empty" { }) } } + +func sensitiveAttribute(t cty.Type) *configschema.Attribute { + return &configschema.Attribute{ + Type: t, + Optional: true, + Sensitive: true, + } +} diff --git a/internal/terraform/context_plan_import_test.go b/internal/terraform/context_plan_import_test.go index c10d60e0c0..91248baca4 100644 --- a/internal/terraform/context_plan_import_test.go +++ b/internal/terraform/context_plan_import_test.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/terraform/internal/configs/configschema" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/providers" + testing_provider "github.com/hashicorp/terraform/internal/providers/testing" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/tfdiags" ) @@ -1413,7 +1414,7 @@ func TestContext2Plan_importGenerateNone(t *testing.T) { import { for_each = [] to = test_object.a - id = "123" + id = "81ba7c97" } `, }) @@ -1437,3 +1438,66 @@ import { t.Fatal("expected no resource changes") } } + +// This is a test for the issue raised in #34992 +func TestContext2Plan_importWithSensitives(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +import { + to = test_object.a + id = "123" +} +`, + }) + + p := &testing_provider.MockProvider{ + GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "test_object": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "sensitive_string": { + Type: cty.String, + Sensitive: true, + Optional: true, + }, + "sensitive_list": { + Type: cty.List(cty.String), + Sensitive: true, + Optional: true, + }, + }, + }, + }, + }, + }, + ImportResourceStateFn: func(request providers.ImportResourceStateRequest) providers.ImportResourceStateResponse { + return providers.ImportResourceStateResponse{ + ImportedResources: []providers.ImportedResource{ + { + TypeName: "test_object", + State: cty.ObjectVal(map[string]cty.Value{ + "sensitive_string": cty.StringVal("sensitive"), + "sensitive_list": cty.ListVal([]cty.Value{cty.StringVal("hello"), cty.StringVal("world")}), + }), + }, + }, + } + }, + } + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + // Just don't crash! + _, diags := ctx.Plan(m, states.NewState(), &PlanOpts{ + Mode: plans.NormalMode, + GenerateConfigPath: "generated.tf", + }) + if diags.HasErrors() { + t.Fatalf("unexpected errors\n%s", diags.Err().Error()) + } +}