From af9dacb9f9059fdc54470edf673b821ae201e601 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 8 Apr 2019 14:42:36 -0400 Subject: [PATCH 1/7] add failing test for diff.Apply Add a diff test using a shcema with ConfigModeAttr. It's in the test provider, because that is what is mostly responsible for exercising diff.Apply, and where the other tests are. --- builtin/providers/test/diff_apply_test.go | 141 ++++++++++++++++++++++ 1 file changed, 141 insertions(+) create mode 100644 builtin/providers/test/diff_apply_test.go diff --git a/builtin/providers/test/diff_apply_test.go b/builtin/providers/test/diff_apply_test.go new file mode 100644 index 000000000..0ee5f4102 --- /dev/null +++ b/builtin/providers/test/diff_apply_test.go @@ -0,0 +1,141 @@ +package test + +import ( + "reflect" + "testing" + + "github.com/hashicorp/terraform/helper/schema" + "github.com/hashicorp/terraform/terraform" +) + +func TestDiffApply_set(t *testing.T) { + priorAttrs := map[string]string{ + "id": "testID", + "egress.#": "1", + "egress.2129912301.cidr_blocks.#": "1", + "egress.2129912301.cidr_blocks.0": "10.0.0.0/8", + "egress.2129912301.description": "Egress description", + "egress.2129912301.from_port": "80", + "egress.2129912301.ipv6_cidr_blocks.#": "0", + "egress.2129912301.prefix_list_ids.#": "0", + "egress.2129912301.protocol": "tcp", + "egress.2129912301.security_groups.#": "0", + "egress.2129912301.self": "false", + "egress.2129912301.to_port": "8000", + } + + diff := &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "egress.2129912301.cidr_blocks.#": {Old: "1", New: "0", NewComputed: false, NewRemoved: false}, + "egress.2129912301.cidr_blocks.0": {Old: "10.0.0.0/8", New: "", NewComputed: false, NewRemoved: true}, + "egress.2129912301.description": {Old: "Egress description", New: "", NewComputed: false, NewRemoved: true}, + "egress.2129912301.from_port": {Old: "80", New: "0", NewComputed: false, NewRemoved: true}, + "egress.2129912301.ipv6_cidr_blocks.#": {Old: "0", New: "0", NewComputed: false, NewRemoved: false}, + "egress.2129912301.prefix_list_ids.#": {Old: "0", New: "0", NewComputed: false, NewRemoved: false}, + "egress.2129912301.protocol": {Old: "tcp", New: "", NewComputed: false, NewRemoved: true}, + "egress.2129912301.security_groups.#": {Old: "0", New: "0", NewComputed: false, NewRemoved: false}, + "egress.2129912301.self": {Old: "false", New: "false", NewComputed: false, NewRemoved: true}, + "egress.2129912301.to_port": {Old: "8000", New: "0", NewComputed: false, NewRemoved: true}, + "egress.746197026.cidr_blocks.#": {Old: "", New: "1", NewComputed: false, NewRemoved: false}, + "egress.746197026.cidr_blocks.0": {Old: "", New: "10.0.0.0/8", NewComputed: false, NewRemoved: false}, + "egress.746197026.description": {Old: "", New: "New egress description", NewComputed: false, NewRemoved: false}, + "egress.746197026.from_port": {Old: "", New: "80", NewComputed: false, NewRemoved: false}, + "egress.746197026.ipv6_cidr_blocks.#": {Old: "", New: "0", NewComputed: false, NewRemoved: false}, + "egress.746197026.prefix_list_ids.#": {Old: "", New: "0", NewComputed: false, NewRemoved: false}, + "egress.746197026.protocol": {Old: "", New: "tcp", NewComputed: false, NewRemoved: false, NewExtra: "tcp"}, + "egress.746197026.security_groups.#": {Old: "", New: "0", NewComputed: false, NewRemoved: false}, + "egress.746197026.self": {Old: "", New: "false", NewComputed: false, NewRemoved: false}, + "egress.746197026.to_port": {Old: "", New: "8000", NewComputed: false, NewRemoved: false}, + }, + } + + resSchema := map[string]*schema.Schema{ + "egress": { + Type: schema.TypeSet, + Optional: true, + Computed: true, + ConfigMode: schema.SchemaConfigModeAttr, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "from_port": { + Type: schema.TypeInt, + Required: true, + }, + + "to_port": { + Type: schema.TypeInt, + Required: true, + }, + + "protocol": { + Type: schema.TypeString, + Required: true, + }, + + "cidr_blocks": { + Type: schema.TypeList, + Optional: true, + Elem: &schema.Schema{ + Type: schema.TypeString, + }, + }, + + "ipv6_cidr_blocks": { + Type: schema.TypeList, + Optional: true, + Elem: &schema.Schema{ + Type: schema.TypeString, + }, + }, + + "prefix_list_ids": { + Type: schema.TypeList, + Optional: true, + Elem: &schema.Schema{Type: schema.TypeString}, + }, + + "security_groups": { + Type: schema.TypeSet, + Optional: true, + Elem: &schema.Schema{Type: schema.TypeString}, + Set: schema.HashString, + }, + + "self": { + Type: schema.TypeBool, + Optional: true, + Default: false, + }, + + "description": { + Type: schema.TypeString, + Optional: true, + }, + }, + }, + }, + } + + expected := map[string]string{ + "egress.#": "1", + "egress.746197026.cidr_blocks.#": "1", + "egress.746197026.cidr_blocks.0": "10.0.0.0/8", + "egress.746197026.description": "New egress description", + "egress.746197026.from_port": "80", "egress.746197026.ipv6_cidr_blocks.#": "0", + "egress.746197026.prefix_list_ids.#": "0", + "egress.746197026.protocol": "tcp", + "egress.746197026.security_groups.#": "0", + "egress.746197026.self": "false", + "egress.746197026.to_port": "8000", + "id": "testID", + } + + attrs, err := diff.Apply(priorAttrs, (&schema.Resource{Schema: resSchema}).CoreConfigSchemaForShimming()) + if err != nil { + t.Fatal(err) + } + + if !reflect.DeepEqual(attrs, expected) { + t.Fatalf("\nexpected: %#v\ngot: %#v\n", expected, attrs) + } +} From 4dbe6add77f0eb912cbd1030047395ffa7ada8c5 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 8 Apr 2019 15:33:35 -0400 Subject: [PATCH 2/7] Revert "helper/schema: Schema.AsSingle flag" This reverts commit 4c0c74571de9c96ad2902ccf4af962ec495cd5d4. --- helper/schema/schema.go | 22 ------------- helper/schema/schema_test.go | 62 ------------------------------------ 2 files changed, 84 deletions(-) diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 15a82e7f7..56f2930fc 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -216,21 +216,8 @@ type Schema struct { // // If the field Optional is set to true then MinItems is ignored and thus // effectively zero. - // - // If MaxItems is 1, you may optionally also set AsSingle in order to have - // Terraform v0.12 or later treat a TypeList or TypeSet as if it were a - // single value. It will remain a list or set in Terraform v0.10 and v0.11. - // Enabling this for an existing attribute after you've made at least one - // v0.12-compatible provider release is a breaking change. AsSingle is - // likely to misbehave when used with deeply-nested set structures due to - // the imprecision of set diffs, so be sure to test it thoroughly, - // including updates that change the set members at all levels. AsSingle - // exists primarily to be used in conjunction with ConfigMode when forcing - // a nested resource to be treated as an attribute, so it can be considered - // an attribute of object type rather than of list/set of object. MaxItems int MinItems int - AsSingle bool // PromoteSingle originally allowed for a single element to be assigned // where a primitive list was expected, but this no longer works from @@ -863,15 +850,6 @@ func (m schemaMap) internalValidate(topSchemaMap schemaMap, attrsOnly bool) erro } } - if v.AsSingle { - if v.MaxItems != 1 { - return fmt.Errorf("%s: MaxItems must be 1 when AsSingle is set", k) - } - if v.Type != TypeList && v.Type != TypeSet { - return fmt.Errorf("%s: AsSingle can be used only with TypeList and TypeSet schemas", k) - } - } - // Computed-only field if v.Computed && !v.Optional { if v.ValidateFunc != nil { diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 327a4e343..4f8d9dd61 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -3924,68 +3924,6 @@ func TestSchemaMap_InternalValidate(t *testing.T) { }, true, // in *schema.Resource with ConfigMode of attribute, so must also have ConfigMode of attribute }, - - "AsSingle okay": { - map[string]*Schema{ - "block": &Schema{ - Type: TypeList, - Optional: true, - MaxItems: 1, - AsSingle: true, - Elem: &Resource{ - Schema: map[string]*Schema{ - "sub": &Schema{ - Type: TypeList, - Optional: true, - Elem: &Resource{}, - }, - }, - }, - }, - }, - false, - }, - - "AsSingle without MaxItems": { - map[string]*Schema{ - "block": &Schema{ - Type: TypeList, - Optional: true, - AsSingle: true, - Elem: &Resource{ - Schema: map[string]*Schema{ - "sub": &Schema{ - Type: TypeList, - Optional: true, - Elem: &Resource{}, - }, - }, - }, - }, - }, - true, // MaxItems must be 1 when AsSingle is set - }, - - "AsSingle on primitive type": { - map[string]*Schema{ - "block": &Schema{ - Type: TypeString, - Optional: true, - MaxItems: 1, - AsSingle: true, - Elem: &Resource{ - Schema: map[string]*Schema{ - "sub": &Schema{ - Type: TypeList, - Optional: true, - Elem: &Resource{}, - }, - }, - }, - }, - }, - true, // Unexpected error occurred: block: MaxItems and MinItems are only supported on lists or sets - }, } for tn, tc := range cases { From 1a9c06d0f5d6ef04ccc18d6d2889b7ff6b9a026a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 8 Apr 2019 15:43:32 -0400 Subject: [PATCH 3/7] Revert "helper/schema: Implementation of the AsSingle mechanism" This reverts commit 1987a92386b05a48d6ba372d6738ab25611ab825. --- builtin/providers/test/provider.go | 1 - builtin/providers/test/resource_as_single.go | 158 ---------- .../providers/test/resource_as_single_test.go | 114 ------- helper/plugin/grpc_provider.go | 35 +-- helper/resource/state_shim.go | 5 +- helper/schema/as_single_fixup.go | 286 ------------------ helper/schema/as_single_fixup_test.go | 239 --------------- helper/schema/core_schema.go | 115 ++----- helper/schema/resource.go | 2 - helper/schema/shims.go | 6 - 10 files changed, 25 insertions(+), 936 deletions(-) delete mode 100644 builtin/providers/test/resource_as_single.go delete mode 100644 builtin/providers/test/resource_as_single_test.go delete mode 100644 helper/schema/as_single_fixup.go delete mode 100644 helper/schema/as_single_fixup_test.go diff --git a/builtin/providers/test/provider.go b/builtin/providers/test/provider.go index e262c2fb5..65a8fdedb 100644 --- a/builtin/providers/test/provider.go +++ b/builtin/providers/test/provider.go @@ -18,7 +18,6 @@ func Provider() terraform.ResourceProvider { }, ResourcesMap: map[string]*schema.Resource{ "test_resource": testResource(), - "test_resource_as_single": testResourceAsSingle(), "test_resource_gh12183": testResourceGH12183(), "test_resource_import_other": testResourceImportOther(), "test_resource_with_custom_diff": testResourceCustomDiff(), diff --git a/builtin/providers/test/resource_as_single.go b/builtin/providers/test/resource_as_single.go deleted file mode 100644 index 1fd640385..000000000 --- a/builtin/providers/test/resource_as_single.go +++ /dev/null @@ -1,158 +0,0 @@ -package test - -import ( - "fmt" - - "github.com/hashicorp/terraform/helper/schema" -) - -func testResourceAsSingle() *schema.Resource { - return &schema.Resource{ - Create: testResourceAsSingleCreate, - Read: testResourceAsSingleRead, - Delete: testResourceAsSingleDelete, - Update: testResourceAsSingleUpdate, - - Schema: map[string]*schema.Schema{ - "list_resource_as_block": { - Type: schema.TypeList, - Optional: true, - MaxItems: 1, - AsSingle: true, - Elem: &schema.Resource{ - Schema: map[string]*schema.Schema{ - "foo": { - Type: schema.TypeString, - Optional: true, - }, - }, - }, - }, - "list_resource_as_attr": { - Type: schema.TypeList, - ConfigMode: schema.SchemaConfigModeAttr, - Optional: true, - MaxItems: 1, - AsSingle: true, - Elem: &schema.Resource{ - Schema: map[string]*schema.Schema{ - "foo": { - Type: schema.TypeString, - Optional: true, - }, - }, - }, - }, - "list_primitive": { - Type: schema.TypeList, - Optional: true, - MaxItems: 1, - AsSingle: true, - Elem: &schema.Schema{ - Type: schema.TypeString, - }, - }, - - "set_resource_as_block": { - Type: schema.TypeSet, - Optional: true, - MaxItems: 1, - AsSingle: true, - Elem: &schema.Resource{ - Schema: map[string]*schema.Schema{ - "foo": { - Type: schema.TypeString, - Optional: true, - }, - }, - }, - }, - "set_resource_as_attr": { - Type: schema.TypeSet, - ConfigMode: schema.SchemaConfigModeAttr, - Optional: true, - MaxItems: 1, - AsSingle: true, - Elem: &schema.Resource{ - Schema: map[string]*schema.Schema{ - "foo": { - Type: schema.TypeString, - Optional: true, - }, - }, - }, - }, - "set_primitive": { - Type: schema.TypeSet, - Optional: true, - MaxItems: 1, - AsSingle: true, - Elem: &schema.Schema{ - Type: schema.TypeString, - }, - }, - }, - } -} - -func testResourceAsSingleCreate(d *schema.ResourceData, meta interface{}) error { - d.SetId("placeholder") - return testResourceAsSingleRead(d, meta) -} - -func testResourceAsSingleRead(d *schema.ResourceData, meta interface{}) error { - for _, k := range []string{"list_resource_as_block", "list_resource_as_attr", "list_primitive"} { - v := d.Get(k) - if v == nil { - continue - } - if l, ok := v.([]interface{}); !ok { - return fmt.Errorf("%s should appear as []interface {}, not %T", k, v) - } else { - for i, item := range l { - switch k { - case "list_primitive": - if _, ok := item.(string); item != nil && !ok { - return fmt.Errorf("%s[%d] should appear as string, not %T", k, i, item) - } - default: - if _, ok := item.(map[string]interface{}); item != nil && !ok { - return fmt.Errorf("%s[%d] should appear as map[string]interface {}, not %T", k, i, item) - } - } - } - } - } - for _, k := range []string{"set_resource_as_block", "set_resource_as_attr", "set_primitive"} { - v := d.Get(k) - if v == nil { - continue - } - if s, ok := v.(*schema.Set); !ok { - return fmt.Errorf("%s should appear as *schema.Set, not %T", k, v) - } else { - for i, item := range s.List() { - switch k { - case "set_primitive": - if _, ok := item.(string); item != nil && !ok { - return fmt.Errorf("%s[%d] should appear as string, not %T", k, i, item) - } - default: - if _, ok := item.(map[string]interface{}); item != nil && !ok { - return fmt.Errorf("%s[%d] should appear as map[string]interface {}, not %T", k, i, item) - } - } - } - } - } - return nil -} - -func testResourceAsSingleUpdate(d *schema.ResourceData, meta interface{}) error { - return testResourceAsSingleRead(d, meta) -} - -func testResourceAsSingleDelete(d *schema.ResourceData, meta interface{}) error { - d.SetId("") - return nil -} diff --git a/builtin/providers/test/resource_as_single_test.go b/builtin/providers/test/resource_as_single_test.go deleted file mode 100644 index 3802d81b0..000000000 --- a/builtin/providers/test/resource_as_single_test.go +++ /dev/null @@ -1,114 +0,0 @@ -package test - -import ( - "strings" - "testing" - - "github.com/hashicorp/terraform/helper/resource" - "github.com/hashicorp/terraform/terraform" -) - -func TestResourceAsSingle(t *testing.T) { - resource.UnitTest(t, resource.TestCase{ - Providers: testAccProviders, - CheckDestroy: testAccCheckResourceDestroy, - Steps: []resource.TestStep{ - resource.TestStep{ - Config: strings.TrimSpace(` -resource "test_resource_as_single" "foo" { - list_resource_as_block { - foo = "as block a" - } - list_resource_as_attr = { - foo = "as attr a" - } - list_primitive = "primitive a" - - set_resource_as_block { - foo = "as block a" - } - set_resource_as_attr = { - foo = "as attr a" - } - set_primitive = "primitive a" -} - `), - Check: resource.ComposeTestCheckFunc( - func(s *terraform.State) error { - t.Log("state after initial create:\n", s.String()) - return nil - }, - resource.TestCheckResourceAttr("test_resource_as_single.foo", "list_resource_as_block.#", "1"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "list_resource_as_block.0.foo", "as block a"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "list_resource_as_attr.#", "1"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "list_resource_as_attr.0.foo", "as attr a"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "list_primitive.#", "1"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "list_primitive.0", "primitive a"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "set_resource_as_block.#", "1"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "set_resource_as_block.1417230722.foo", "as block a"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "set_resource_as_attr.#", "1"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "set_resource_as_attr.2549052262.foo", "as attr a"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "set_primitive.#", "1"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "set_primitive.247272358", "primitive a"), - ), - }, - resource.TestStep{ - Config: strings.TrimSpace(` -resource "test_resource_as_single" "foo" { - list_resource_as_block { - foo = "as block b" - } - list_resource_as_attr = { - foo = "as attr b" - } - list_primitive = "primitive b" - - set_resource_as_block { - foo = "as block b" - } - set_resource_as_attr = { - foo = "as attr b" - } - set_primitive = "primitive b" -} - `), - Check: resource.ComposeTestCheckFunc( - func(s *terraform.State) error { - t.Log("state after update:\n", s.String()) - return nil - }, - resource.TestCheckResourceAttr("test_resource_as_single.foo", "list_resource_as_block.#", "1"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "list_resource_as_block.0.foo", "as block b"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "list_resource_as_attr.#", "1"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "list_resource_as_attr.0.foo", "as attr b"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "list_primitive.#", "1"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "list_primitive.0", "primitive b"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "set_resource_as_block.#", "1"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "set_resource_as_block.2136238657.foo", "as block b"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "set_resource_as_attr.#", "1"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "set_resource_as_attr.3166838949.foo", "as attr b"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "set_primitive.#", "1"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "set_primitive.630210661", "primitive b"), - ), - }, - resource.TestStep{ - Config: strings.TrimSpace(` -resource "test_resource_as_single" "foo" { -} - `), - Check: resource.ComposeTestCheckFunc( - func(s *terraform.State) error { - t.Log("state after everything unset:\n", s.String()) - return nil - }, - resource.TestCheckResourceAttr("test_resource_as_single.foo", "list_resource_as_block.#", "0"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "list_resource_as_attr.#", "0"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "list_primitive.#", "0"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "set_resource_as_block.#", "0"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "set_resource_as_attr.#", "0"), - resource.TestCheckResourceAttr("test_resource_as_single.foo", "set_primitive.#", "0"), - ), - }, - }, - }) -} diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index de90d88ac..09b5f9424 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -275,12 +275,6 @@ func (s *GRPCProviderServer) UpgradeResourceState(_ context.Context, req *proto. } } - // NOTE WELL: The AsSingle mechanism cannot be automatically normalized here, - // so providers that use it must be ready to handle both normalized and - // unnormalized input in their upgrade codepaths. The _result_ of an upgrade - // should set a single-element list/set for any AsSingle element so that it - // can be normalized to a single value automatically on return. - // We first need to upgrade a flatmap state if it exists. // There should never be both a JSON and Flatmap state in the request. if req.RawState.Flatmap != nil { @@ -298,8 +292,6 @@ func (s *GRPCProviderServer) UpgradeResourceState(_ context.Context, req *proto. return resp, nil } - schema.FixupAsSingleConfigValueOut(jsonMap, s.provider.ResourcesMap[req.TypeName].Schema) - // now we need to turn the state into the default json representation, so // that it can be re-decoded using the actual schema. val, err := schema.JSONMapToStateValue(jsonMap, blockForShimming) @@ -468,7 +460,6 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil } - // res.ShimInstanceStateFromValue result has already had FixupAsSingleInstanceStateIn applied newInstanceState, err := res.RefreshWithoutUpgrade(instanceState, s.provider.Meta()) if err != nil { @@ -560,7 +551,6 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil } - // res.ShimInstanceStateFromValue result has already had FixupAsSingleInstanceStateIn applied priorPrivate := make(map[string]interface{}) if len(req.PriorPrivate) > 0 { if err := json.Unmarshal(req.PriorPrivate, &priorPrivate); err != nil { @@ -612,17 +602,7 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl } // now we need to apply the diff to the prior state, so get the planned state - plannedAttrs, err := diff.Apply(priorState.Attributes, res.CoreConfigSchemaWhenShimmed()) - schema.FixupAsSingleInstanceStateOut( - &terraform.InstanceState{Attributes: plannedAttrs}, - s.provider.ResourcesMap[req.TypeName], - ) - - // We also fix up the diff for AsSingle here, but note that we intentionally - // do it _after_ diff.Apply (so that the state can have its own fixup applied) - // but before we deal with requiresNew below so that fixing up the diff - // also fixes up the requiresNew keys to match. - schema.FixupAsSingleInstanceDiffOut(diff, s.provider.ResourcesMap[req.TypeName]) + plannedAttrs, err := diff.Apply(priorState.Attributes, block) plannedStateVal, err := hcl2shim.HCL2ValueFromFlatmap(plannedAttrs, blockForShimming.ImpliedType()) if err != nil { @@ -763,7 +743,6 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil } - // res.ShimInstanceStateFromValue result has already had FixupAsSingleInstanceStateIn applied private := make(map[string]interface{}) if len(req.PlannedPrivate) > 0 { @@ -785,10 +764,7 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A Destroy: true, } } else { - diff, err = schema.DiffFromValues( - priorStateVal, plannedStateVal, - stripResourceModifiers(res), - ) + diff, err = schema.DiffFromValues(priorStateVal, plannedStateVal, stripResourceModifiers(res)) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -802,10 +778,6 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A } } - // NOTE WELL: schema.DiffFromValues has already effectively applied - // schema.FixupAsSingleInstanceDiffIn to the diff, so we need not (and must not) - // repeat that here. - // We need to fix any sets that may be using the "~" index prefix to // indicate partially computed. The special sigil isn't really used except // as a clue to visually indicate that the set isn't wholly known. @@ -934,8 +906,6 @@ func (s *GRPCProviderServer) ImportResourceState(_ context.Context, req *proto.I // copy the ID again just to be sure it wasn't missed is.Attributes["id"] = is.ID - schema.FixupAsSingleInstanceStateOut(is, s.provider.ResourcesMap[req.TypeName]) - resourceType := is.Ephemeral.Type if resourceType == "" { resourceType = req.TypeName @@ -1014,7 +984,6 @@ func (s *GRPCProviderServer) ReadDataSource(_ context.Context, req *proto.ReadDa resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil } - schema.FixupAsSingleInstanceStateOut(newInstanceState, s.provider.DataSourcesMap[req.TypeName]) newStateVal, err := schema.StateValueFromInstanceState(newInstanceState, blockForShimming.ImpliedType()) if err != nil { diff --git a/helper/resource/state_shim.go b/helper/resource/state_shim.go index 9b08b8ab5..b2aff99d1 100644 --- a/helper/resource/state_shim.go +++ b/helper/resource/state_shim.go @@ -4,11 +4,13 @@ import ( "fmt" "github.com/hashicorp/terraform/addrs" + "github.com/zclconf/go-cty/cty" + "github.com/hashicorp/terraform/config/hcl2shim" "github.com/hashicorp/terraform/helper/schema" + "github.com/hashicorp/terraform/states" "github.com/hashicorp/terraform/terraform" - "github.com/zclconf/go-cty/cty" ) // shimState takes a new *states.State and reverts it to a legacy state for the provider ACC tests @@ -153,7 +155,6 @@ func shimmedAttributes(instance *states.ResourceInstanceObjectSrc, res *schema.R } instanceState, err := res.ShimInstanceStateFromValue(rio.Value) - // instanceState has already had the AsSingle fixup applied because ShimInstanceStateFromValue calls FixupAsSingleInstanceStateIn. if err != nil { return nil, err } diff --git a/helper/schema/as_single_fixup.go b/helper/schema/as_single_fixup.go deleted file mode 100644 index 95deab318..000000000 --- a/helper/schema/as_single_fixup.go +++ /dev/null @@ -1,286 +0,0 @@ -package schema - -import ( - "sort" - "strings" - - "github.com/hashicorp/terraform/terraform" -) - -// FixupAsSingleResourceConfigIn modifies the given ResourceConfig in-place if -// any attributes in the schema have the AsSingle flag set, wrapping the given -// values for these in an extra level of slice so that they can be understood -// by legacy SDK code that'll be expecting to decode into a list/set. -func FixupAsSingleResourceConfigIn(rc *terraform.ResourceConfig, s map[string]*Schema) { - if rc == nil { - return - } - FixupAsSingleConfigValueIn(rc.Config, s) -} - -// FixupAsSingleInstanceStateIn modifies the given InstanceState in-place if -// any attributes in the schema have the AsSingle flag set, adding additional -// index steps to the flatmap keys for these so that they can be understood -// by legacy SDK code that'll be expecting to decode into a list/set. -func FixupAsSingleInstanceStateIn(is *terraform.InstanceState, r *Resource) { - fixupAsSingleInstanceState(is, r.Schema, "", fixupAsSingleFlatmapKeysIn) -} - -// FixupAsSingleInstanceStateOut modifies the given InstanceState in-place if -// any attributes in the schema have the AsSingle flag set, removing unneeded -// index steps from the flatmap keys for these so that they can be understood -// by the shim back to Terraform Core as a single nested value. -func FixupAsSingleInstanceStateOut(is *terraform.InstanceState, r *Resource) { - fixupAsSingleInstanceState(is, r.Schema, "", fixupAsSingleFlatmapKeysOut) -} - -// FixupAsSingleInstanceDiffIn modifies the given InstanceDiff in-place if any -// attributes in the schema have the AsSingle flag set, adding additional index -// steps to the flatmap keys for these so that they can be understood by legacy -// SDK code that'll be expecting to decode into a list/set. -func FixupAsSingleInstanceDiffIn(id *terraform.InstanceDiff, r *Resource) { - fixupAsSingleInstanceDiff(id, r.Schema, "", fixupAsSingleAttrsMapKeysIn) -} - -// FixupAsSingleInstanceDiffOut modifies the given InstanceDiff in-place if any -// attributes in the schema have the AsSingle flag set, removing unneeded index -// steps from the flatmap keys for these so that they can be understood by the -// shim back to Terraform Core as a single nested value. -func FixupAsSingleInstanceDiffOut(id *terraform.InstanceDiff, r *Resource) { - fixupAsSingleInstanceDiff(id, r.Schema, "", fixupAsSingleAttrsMapKeysOut) -} - -// FixupAsSingleConfigValueIn modifies the given "config value" in-place if -// any attributes in the schema have the AsSingle flag set, wrapping the given -// values for these in an extra level of slice so that they can be understood -// by legacy SDK code that'll be expecting to decode into a list/set. -// -// "Config value" for the purpose of this function has the same meaning as for -// the hcl2shims: a map[string]interface{} using the same subset of Go value -// types that would be generated by HCL/HIL when decoding a configuration in -// Terraform v0.11. -func FixupAsSingleConfigValueIn(c map[string]interface{}, s map[string]*Schema) { - for k, as := range s { - if !as.AsSingle { - continue // Don't touch non-AsSingle values at all. This is explicitly opt-in. - } - - v, ok := c[k] - if ok { - c[k] = []interface{}{v} - } - - if nr, ok := as.Elem.(*Resource); ok { - // Recursively fixup nested attributes too - nm, ok := v.(map[string]interface{}) - if !ok { - // Weird for a nested resource to not be a map, but we'll tolerate it rather than crashing - continue - } - FixupAsSingleConfigValueIn(nm, nr.Schema) - } - } -} - -// FixupAsSingleConfigValueOut modifies the given "config value" in-place if -// any attributes in the schema have the AsSingle flag set, unwrapping the -// given values from their single-element slices so that they can be understood -// as a single object value by Terraform Core. -// -// This is the opposite of fixupAsSingleConfigValueIn. -func FixupAsSingleConfigValueOut(c map[string]interface{}, s map[string]*Schema) { - for k, as := range s { - if !as.AsSingle { - continue // Don't touch non-AsSingle values at all. This is explicitly opt-in. - } - - sv, ok := c[k].([]interface{}) - if ok && len(sv) != 0 { // Should always be a single-element slice, but if not we'll just leave it alone rather than crashing - c[k] = sv[0] - if nr, ok := as.Elem.(*Resource); ok { - // Recursively fixup nested attributes too - nm, ok := sv[0].(map[string]interface{}) - if ok { - FixupAsSingleConfigValueOut(nm, nr.Schema) - } - } - } - } -} - -func fixupAsSingleInstanceState(is *terraform.InstanceState, s map[string]*Schema, prefix string, fn func(map[string]string, string) string) { - if is == nil { - return - } - - for k, as := range s { - if !as.AsSingle { - continue // Don't touch non-AsSingle values at all. This is explicitly opt-in. - } - - nextPrefix := fn(is.Attributes, prefix+k+".") - if nr, ok := as.Elem.(*Resource); ok { - // Recursively fixup nested attributes too - fixupAsSingleInstanceState(is, nr.Schema, nextPrefix, fn) - } - } -} - -func fixupAsSingleInstanceDiff(id *terraform.InstanceDiff, s map[string]*Schema, prefix string, fn func(map[string]*terraform.ResourceAttrDiff, string) string) { - if id == nil { - return - } - - for k, as := range s { - if !as.AsSingle { - continue // Don't touch non-AsSingle values at all. This is explicitly opt-in. - } - - nextPrefix := fn(id.Attributes, prefix+k+".") - if nr, ok := as.Elem.(*Resource); ok { - // Recursively fixup nested attributes too - fixupAsSingleInstanceDiff(id, nr.Schema, nextPrefix, fn) - } - } -} - -// fixupAsSingleFlatmapKeysIn searches the given flatmap for all keys with -// the given prefix (which must end with a dot) and replaces them with keys -// where that prefix is followed by the dummy index "0." and, if any such -// keys are found, a ".#"-suffixed key is also added whose value is "1". -// -// This function will also replace an exact match of the given prefix with -// the trailing dot removed, to recognize values of primitive-typed attributes. -func fixupAsSingleFlatmapKeysIn(attrs map[string]string, prefix string) string { - ks := make([]string, 0, len(attrs)) - for k := range attrs { - ks = append(ks, k) - } - sort.Strings(ks) // Makes no difference for valid input, but will ensure we handle invalid input deterministically - - for _, k := range ks { - newK, countK := fixupAsSingleFlatmapKeyIn(k, prefix) - if _, exists := attrs[newK]; k != newK && !exists { - attrs[newK] = attrs[k] - delete(attrs, k) - } - if _, exists := attrs[countK]; countK != "" && !exists { - attrs[countK] = "1" - } - } - - return prefix + "0." -} - -// fixupAsSingleAttrsMapKeysIn searches the given AttrDiff map for all keys with -// the given prefix (which must end with a dot) and replaces them with keys -// where that prefix is followed by the dummy index "0." and, if any such -// keys are found, a ".#"-suffixed key is also added whose value is "1". -// -// This function will also replace an exact match of the given prefix with -// the trailing dot removed, to recognize values of primitive-typed attributes. -func fixupAsSingleAttrsMapKeysIn(attrs map[string]*terraform.ResourceAttrDiff, prefix string) string { - ks := make([]string, 0, len(attrs)) - for k := range attrs { - ks = append(ks, k) - } - sort.Strings(ks) // Makes no difference for valid input, but will ensure we handle invalid input deterministically - - for _, k := range ks { - newK, countK := fixupAsSingleFlatmapKeyIn(k, prefix) - if _, exists := attrs[newK]; k != newK && !exists { - attrs[newK] = attrs[k] - delete(attrs, k) - } - if _, exists := attrs[countK]; countK != "" && !exists { - attrs[countK] = &terraform.ResourceAttrDiff{ - Old: "1", // One should _always_ be present, so this seems okay? - New: "1", - } - } - } - - return prefix + "0." -} - -func fixupAsSingleFlatmapKeyIn(k, prefix string) (string, string) { - exact := prefix[:len(prefix)-1] - - switch { - case k == exact: - return exact + ".0", exact + ".#" - case strings.HasPrefix(k, prefix): - return prefix + "0." + k[len(prefix):], prefix + "#" - default: - return k, "" - } -} - -// fixupAsSingleFlatmapKeysOut searches the given flatmap for all keys with -// the given prefix (which must end with a dot) and replaces them with keys -// where the following dot-separated label is removed, under the assumption that -// it's an index that is no longer needed and, if such a key is present, also -// remove the "count" key for the prefix, which is the prefix followed by "#". -func fixupAsSingleFlatmapKeysOut(attrs map[string]string, prefix string) string { - ks := make([]string, 0, len(attrs)) - for k := range attrs { - ks = append(ks, k) - } - sort.Strings(ks) // Makes no difference for valid input, but will ensure we handle invalid input deterministically - - for _, k := range ks { - newK := fixupAsSingleFlatmapKeyOut(k, prefix) - if newK != k && newK == "" { - delete(attrs, k) - } else if _, exists := attrs[newK]; newK != k && !exists { - attrs[newK] = attrs[k] - delete(attrs, k) - } - } - - delete(attrs, prefix+"#") // drop the count key, if it's present - return prefix -} - -// fixupAsSingleAttrsMapKeysOut searches the given AttrDiff map for all keys with -// the given prefix (which must end with a dot) and replaces them with keys -// where the following dot-separated label is removed, under the assumption that -// it's an index that is no longer needed and, if such a key is present, also -// remove the "count" key for the prefix, which is the prefix followed by "#". -func fixupAsSingleAttrsMapKeysOut(attrs map[string]*terraform.ResourceAttrDiff, prefix string) string { - ks := make([]string, 0, len(attrs)) - for k := range attrs { - ks = append(ks, k) - } - sort.Strings(ks) // Makes no difference for valid input, but will ensure we handle invalid input deterministically - - for _, k := range ks { - newK := fixupAsSingleFlatmapKeyOut(k, prefix) - if newK != k && newK == "" { - delete(attrs, k) - } else if _, exists := attrs[newK]; newK != k && !exists { - attrs[newK] = attrs[k] - delete(attrs, k) - } - } - - delete(attrs, prefix+"#") // drop the count key, if it's present - return prefix -} - -func fixupAsSingleFlatmapKeyOut(k, prefix string) string { - if strings.HasPrefix(k, prefix) { - remain := k[len(prefix):] - if remain == "#" { - // Don't need the count element anymore - return "" - } - dotIdx := strings.Index(remain, ".") - if dotIdx == -1 { - return prefix[:len(prefix)-1] // no follow-on attributes then - } else { - return prefix + remain[dotIdx+1:] // everything after the next dot - } - } - return k -} diff --git a/helper/schema/as_single_fixup_test.go b/helper/schema/as_single_fixup_test.go deleted file mode 100644 index deb43095f..000000000 --- a/helper/schema/as_single_fixup_test.go +++ /dev/null @@ -1,239 +0,0 @@ -package schema - -import ( - "testing" - - "github.com/google/go-cmp/cmp" - "github.com/hashicorp/terraform/terraform" -) - -func TestFixupAsSingleInstanceStateInOut(t *testing.T) { - tests := map[string]struct { - AttrsIn map[string]string - AttrsOut map[string]string - Schema map[string]*Schema - }{ - "empty": { - nil, - nil, - nil, - }, - "simple": { - map[string]string{ - "a": "a value", - }, - map[string]string{ - "a": "a value", - }, - map[string]*Schema{ - "a": {Type: TypeString, Optional: true}, - }, - }, - "normal list of primitive, empty": { - map[string]string{ - "a.%": "0", - }, - map[string]string{ - "a.%": "0", - }, - map[string]*Schema{ - "a": { - Type: TypeList, - Optional: true, - Elem: &Schema{Type: TypeString}, - }, - }, - }, - "normal list of primitive, single": { - map[string]string{ - "a.%": "1", - "a.0": "hello", - }, - map[string]string{ - "a.%": "1", - "a.0": "hello", - }, - map[string]*Schema{ - "a": { - Type: TypeList, - Optional: true, - Elem: &Schema{Type: TypeString}, - }, - }, - }, - "AsSingle list of primitive": { - map[string]string{ - "a": "hello", - }, - map[string]string{ - "a.#": "1", - "a.0": "hello", - }, - map[string]*Schema{ - "a": { - Type: TypeList, - Optional: true, - MaxItems: 1, - AsSingle: true, - Elem: &Schema{Type: TypeString}, - }, - }, - }, - "AsSingle list of resource": { - map[string]string{ - "a.b": "hello", - }, - map[string]string{ - "a.#": "1", - "a.0.b": "hello", - }, - map[string]*Schema{ - "a": { - Type: TypeList, - Optional: true, - MaxItems: 1, - AsSingle: true, - Elem: &Resource{ - Schema: map[string]*Schema{ - "b": { - Type: TypeString, - Optional: true, - }, - }, - }, - }, - }, - }, - "AsSingle list of resource with nested primitive list": { - map[string]string{ - "a.b.#": "1", - "a.b.0": "hello", - }, - map[string]string{ - "a.#": "1", - "a.0.b.#": "1", - "a.0.b.0": "hello", - }, - map[string]*Schema{ - "a": { - Type: TypeList, - Optional: true, - MaxItems: 1, - AsSingle: true, - Elem: &Resource{ - Schema: map[string]*Schema{ - "b": { - Type: TypeList, - Optional: true, - Elem: &Schema{Type: TypeString}, - }, - }, - }, - }, - }, - }, - "AsSingle list of resource with nested AsSingle primitive list": { - map[string]string{ - "a.b": "hello", - }, - map[string]string{ - "a.#": "1", - "a.0.b.#": "1", - "a.0.b.0": "hello", - }, - map[string]*Schema{ - "a": { - Type: TypeList, - Optional: true, - MaxItems: 1, - AsSingle: true, - Elem: &Resource{ - Schema: map[string]*Schema{ - "b": { - Type: TypeList, - Optional: true, - MaxItems: 1, - AsSingle: true, - Elem: &Schema{Type: TypeString}, - }, - }, - }, - }, - }, - }, - "AsSingle list of resource with nested AsSingle resource list": { - map[string]string{ - "a.b.c": "hello", - }, - map[string]string{ - "a.#": "1", - "a.0.b.#": "1", - "a.0.b.0.c": "hello", - }, - map[string]*Schema{ - "a": { - Type: TypeList, - Optional: true, - MaxItems: 1, - AsSingle: true, - Elem: &Resource{ - Schema: map[string]*Schema{ - "b": { - Type: TypeList, - Optional: true, - MaxItems: 1, - AsSingle: true, - Elem: &Resource{ - Schema: map[string]*Schema{ - "c": { - Type: TypeString, - Optional: true, - }, - }, - }, - }, - }, - }, - }, - }, - }, - } - - copyMap := func(m map[string]string) map[string]string { - if m == nil { - return nil - } - ret := make(map[string]string, len(m)) - for k, v := range m { - ret[k] = v - } - return ret - } - - for name, test := range tests { - t.Run(name, func(t *testing.T) { - t.Run("In", func(t *testing.T) { - input := copyMap(test.AttrsIn) - is := &terraform.InstanceState{ - Attributes: input, - } - r := &Resource{Schema: test.Schema} - FixupAsSingleInstanceStateIn(is, r) - if !cmp.Equal(is.Attributes, test.AttrsOut) { - t.Errorf("wrong result\ninput: %#v\ngot: %#v\nwant: %#v\n\n%s", input, is.Attributes, test.AttrsOut, cmp.Diff(test.AttrsOut, is.Attributes)) - } - }) - t.Run("Out", func(t *testing.T) { - input := copyMap(test.AttrsOut) - is := &terraform.InstanceState{ - Attributes: input, - } - r := &Resource{Schema: test.Schema} - FixupAsSingleInstanceStateOut(is, r) - if !cmp.Equal(is.Attributes, test.AttrsIn) { - t.Errorf("wrong result\ninput: %#v\ngot: %#v\nwant: %#v\n\n%s", input, is.Attributes, test.AttrsIn, cmp.Diff(test.AttrsIn, is.Attributes)) - } - }) - }) - } -} diff --git a/helper/schema/core_schema.go b/helper/schema/core_schema.go index 8fb01358d..875677eb3 100644 --- a/helper/schema/core_schema.go +++ b/helper/schema/core_schema.go @@ -22,35 +22,6 @@ import ( // This method presumes a schema that passes InternalValidate, and so may // panic or produce an invalid result if given an invalid schemaMap. func (m schemaMap) CoreConfigSchema() *configschema.Block { - return m.coreConfigSchema(true, true) -} - -// CoreConfigSchemaForShimming is a variant of CoreConfigSchema that returns -// the schema that should be used when applying our shimming behaviors. -// -// In particular, it ignores the SkipCoreTypeCheck flag on any legacy schemas, -// since the shims live on the SDK side and so they need to see the full -// type information that we'd normally hide from Terraform Core when skipping -// type checking over there. -func (m schemaMap) CoreConfigSchemaForShimming() *configschema.Block { - return m.coreConfigSchema(true, false) -} - -// CoreConfigSchemaWhenShimmed is a variant of CoreConfigSchema that returns -// the schema as it would appear when working with data structures that have -// already been shimmed to the legacy form. -// -// In particular, it ignores the AsSingle flag on any legacy schemas and behaves -// as if they were really lists/sets instead, thus giving a description of -// the shape of the data structure after the AsSingle fixup has been applied. -// -// This should be used with care only in unusual situations where we need to -// work with an already-shimmed value using a new-style schema. -func (m schemaMap) CoreConfigSchemaWhenShimmed() *configschema.Block { - return m.coreConfigSchema(false, false) -} - -func (m schemaMap) coreConfigSchema(asSingle, skipCoreCheck bool) *configschema.Block { if len(m) == 0 { // We return an actual (empty) object here, rather than a nil, // because a nil result would mean that we don't have a schema at @@ -65,7 +36,7 @@ func (m schemaMap) coreConfigSchema(asSingle, skipCoreCheck bool) *configschema. for name, schema := range m { if schema.Elem == nil { - ret.Attributes[name] = schema.coreConfigSchemaAttribute(asSingle, skipCoreCheck) + ret.Attributes[name] = schema.coreConfigSchemaAttribute() continue } if schema.Type == TypeMap { @@ -79,27 +50,27 @@ func (m schemaMap) coreConfigSchema(asSingle, skipCoreCheck bool) *configschema. sch.Elem = &Schema{ Type: TypeString, } - ret.Attributes[name] = sch.coreConfigSchemaAttribute(asSingle, skipCoreCheck) + ret.Attributes[name] = sch.coreConfigSchemaAttribute() continue } } switch schema.ConfigMode { case SchemaConfigModeAttr: - ret.Attributes[name] = schema.coreConfigSchemaAttribute(asSingle, skipCoreCheck) + ret.Attributes[name] = schema.coreConfigSchemaAttribute() case SchemaConfigModeBlock: - ret.BlockTypes[name] = schema.coreConfigSchemaBlock(asSingle, skipCoreCheck) + ret.BlockTypes[name] = schema.coreConfigSchemaBlock() default: // SchemaConfigModeAuto, or any other invalid value if schema.Computed && !schema.Optional { // Computed-only schemas are always handled as attributes, // because they never appear in configuration. - ret.Attributes[name] = schema.coreConfigSchemaAttribute(asSingle, skipCoreCheck) + ret.Attributes[name] = schema.coreConfigSchemaAttribute() continue } switch schema.Elem.(type) { case *Schema, ValueType: - ret.Attributes[name] = schema.coreConfigSchemaAttribute(asSingle, skipCoreCheck) + ret.Attributes[name] = schema.coreConfigSchemaAttribute() case *Resource: - ret.BlockTypes[name] = schema.coreConfigSchemaBlock(asSingle, skipCoreCheck) + ret.BlockTypes[name] = schema.coreConfigSchemaBlock() default: // Should never happen for a valid schema panic(fmt.Errorf("invalid Schema.Elem %#v; need *Schema or *Resource", schema.Elem)) @@ -114,7 +85,7 @@ func (m schemaMap) coreConfigSchema(asSingle, skipCoreCheck bool) *configschema. // of a schema. This is appropriate only for primitives or collections whose // Elem is an instance of Schema. Use coreConfigSchemaBlock for collections // whose elem is a whole resource. -func (s *Schema) coreConfigSchemaAttribute(asSingle, skipCoreCheck bool) *configschema.Attribute { +func (s *Schema) coreConfigSchemaAttribute() *configschema.Attribute { // The Schema.DefaultFunc capability adds some extra weirdness here since // it can be combined with "Required: true" to create a sitution where // required-ness is conditional. Terraform Core doesn't share this concept, @@ -145,7 +116,7 @@ func (s *Schema) coreConfigSchemaAttribute(asSingle, skipCoreCheck bool) *config } return &configschema.Attribute{ - Type: s.coreConfigSchemaType(asSingle, skipCoreCheck), + Type: s.coreConfigSchemaType(), Optional: opt, Required: reqd, Computed: s.Computed, @@ -157,9 +128,9 @@ func (s *Schema) coreConfigSchemaAttribute(asSingle, skipCoreCheck bool) *config // coreConfigSchemaBlock prepares a configschema.NestedBlock representation of // a schema. This is appropriate only for collections whose Elem is an instance // of Resource, and will panic otherwise. -func (s *Schema) coreConfigSchemaBlock(asSingle, skipCoreCheck bool) *configschema.NestedBlock { +func (s *Schema) coreConfigSchemaBlock() *configschema.NestedBlock { ret := &configschema.NestedBlock{} - if nested := schemaMap(s.Elem.(*Resource).Schema).coreConfigSchema(asSingle, skipCoreCheck); nested != nil { + if nested := s.Elem.(*Resource).coreConfigSchema(); nested != nil { ret.Block = *nested } switch s.Type { @@ -177,14 +148,6 @@ func (s *Schema) coreConfigSchemaBlock(asSingle, skipCoreCheck bool) *configsche ret.MinItems = s.MinItems ret.MaxItems = s.MaxItems - if s.AsSingle && asSingle { - // In AsSingle mode, we artifically force a TypeList or TypeSet - // attribute in the SDK to be treated as a single block by Terraform Core. - // This must then be fixed up in the shim code (in helper/plugin) so - // that the SDK still sees the lists or sets it's expecting. - ret.Nesting = configschema.NestingSingle - } - if s.Required && s.MinItems == 0 { // configschema doesn't have a "required" representation for nested // blocks, but we can fake it by requiring at least one item. @@ -210,15 +173,7 @@ func (s *Schema) coreConfigSchemaBlock(asSingle, skipCoreCheck bool) *configsche // coreConfigSchemaType determines the core config schema type that corresponds // to a particular schema's type. -func (s *Schema) coreConfigSchemaType(asSingle, skipCoreCheck bool) cty.Type { - if skipCoreCheck && s.SkipCoreTypeCheck { - // If we're preparing a schema for Terraform Core and the schema is - // asking us to skip the Core type-check then we'll tell core that this - // attribute is dynamically-typed, so it'll just pass through anything - // and let us validate it on the plugin side. - return cty.DynamicPseudoType - } - +func (s *Schema) coreConfigSchemaType() cty.Type { switch s.Type { case TypeString: return cty.String @@ -233,17 +188,17 @@ func (s *Schema) coreConfigSchemaType(asSingle, skipCoreCheck bool) cty.Type { var elemType cty.Type switch set := s.Elem.(type) { case *Schema: - elemType = set.coreConfigSchemaType(asSingle, skipCoreCheck) + elemType = set.coreConfigSchemaType() case ValueType: // This represents a mistake in the provider code, but it's a // common one so we'll just shim it. - elemType = (&Schema{Type: set}).coreConfigSchemaType(asSingle, skipCoreCheck) + elemType = (&Schema{Type: set}).coreConfigSchemaType() case *Resource: // By default we construct a NestedBlock in this case, but this // behavior is selected either for computed-only schemas or // when ConfigMode is explicitly SchemaConfigModeBlock. // See schemaMap.CoreConfigSchema for the exact rules. - elemType = schemaMap(set.Schema).coreConfigSchema(asSingle, skipCoreCheck).ImpliedType() + elemType = set.coreConfigSchema().ImpliedType() default: if set != nil { // Should never happen for a valid schema @@ -253,13 +208,6 @@ func (s *Schema) coreConfigSchemaType(asSingle, skipCoreCheck bool) cty.Type { // to be compatible with them. elemType = cty.String } - if s.AsSingle && asSingle { - // In AsSingle mode, we artifically force a TypeList or TypeSet - // attribute in the SDK to be treated as a single value by Terraform Core. - // This must then be fixed up in the shim code (in helper/plugin) so - // that the SDK still sees the lists or sets it's expecting. - return elemType - } switch s.Type { case TypeList: return cty.List(elemType) @@ -281,34 +229,7 @@ func (s *Schema) coreConfigSchemaType(asSingle, skipCoreCheck bool) cty.Type { // the resource's schema. CoreConfigSchema adds the implicitly required "id" // attribute for top level resources if it doesn't exist. func (r *Resource) CoreConfigSchema() *configschema.Block { - return r.coreConfigSchema(true, true) -} - -// CoreConfigSchemaForShimming is a variant of CoreConfigSchema that returns -// the schema that should be used to apply shims on the SDK side. -// -// In particular, it ignores the SkipCoreTypeCheck flag on any legacy schemas -// and uses the real type information instead. -func (r *Resource) CoreConfigSchemaForShimming() *configschema.Block { - return r.coreConfigSchema(true, false) -} - -// CoreConfigSchemaWhenShimmed is a variant of CoreConfigSchema that returns -// the schema as it would appear when working with data structures that have -// already been shimmed to the legacy form. -// -// In particular, it ignores the AsSingle flag on any legacy schemas and behaves -// as if they were really lists/sets instead, thus giving a description of -// the shape of the data structure after the AsSingle fixup has been applied. -// -// This should be used with care only in unusual situations where we need to -// work with an already-shimmed value using a new-style schema. -func (r *Resource) CoreConfigSchemaWhenShimmed() *configschema.Block { - return r.coreConfigSchema(false, false) -} - -func (r *Resource) coreConfigSchema(asSingle, skipCoreCheck bool) *configschema.Block { - block := schemaMap(r.Schema).coreConfigSchema(asSingle, skipCoreCheck) + block := r.coreConfigSchema() if block.Attributes == nil { block.Attributes = map[string]*configschema.Attribute{} @@ -377,6 +298,10 @@ func (r *Resource) coreConfigSchema(asSingle, skipCoreCheck bool) *configschema. return block } +func (r *Resource) coreConfigSchema() *configschema.Block { + return schemaMap(r.Schema).CoreConfigSchema() +} + // CoreConfigSchema is a convenient shortcut for calling CoreConfigSchema // on the backends's schema. func (r *Backend) CoreConfigSchema() *configschema.Block { diff --git a/helper/schema/resource.go b/helper/schema/resource.go index d65743c96..d96bbcfde 100644 --- a/helper/schema/resource.go +++ b/helper/schema/resource.go @@ -162,8 +162,6 @@ func (r *Resource) ShimInstanceStateFromValue(state cty.Value) (*terraform.Insta // match those from the Schema. s := terraform.NewInstanceStateShimmedFromValue(state, r.SchemaVersion) - FixupAsSingleInstanceStateIn(s, r) - // We now rebuild the state through the ResourceData, so that the set indexes // match what helper/schema expects. data, err := schemaMap(r.Schema).Data(s, nil) diff --git a/helper/schema/shims.go b/helper/schema/shims.go index 4dac6eb49..d99fb3965 100644 --- a/helper/schema/shims.go +++ b/helper/schema/shims.go @@ -14,10 +14,6 @@ import ( // derives a terraform.InstanceDiff to give to the legacy providers. This is // used to take the states provided by the new ApplyResourceChange method and // convert them to a state+diff required for the legacy Apply method. -// -// If the fixup function is non-nil, it will be called with the constructed -// shimmed InstanceState and ResourceConfig values to do any necessary in-place -// mutations before producing the diff. func DiffFromValues(prior, planned cty.Value, res *Resource) (*terraform.InstanceDiff, error) { return diffFromValues(prior, planned, res, nil) } @@ -28,7 +24,6 @@ func DiffFromValues(prior, planned cty.Value, res *Resource) (*terraform.Instanc // have already been done. func diffFromValues(prior, planned cty.Value, res *Resource, cust CustomizeDiffFunc) (*terraform.InstanceDiff, error) { instanceState, err := res.ShimInstanceStateFromValue(prior) - // The result of ShimInstanceStateFromValue already has FixupAsSingleInstanceStateIn applied if err != nil { return nil, err } @@ -36,7 +31,6 @@ func diffFromValues(prior, planned cty.Value, res *Resource, cust CustomizeDiffF configSchema := res.CoreConfigSchema() cfg := terraform.NewResourceConfigShimmed(planned, configSchema) - FixupAsSingleResourceConfigIn(cfg, schemaMap(res.Schema)) diff, err := schemaMap(res.Schema).Diff(instanceState, cfg, cust, nil, false) if err != nil { From c5023c7702e5d75183f2584800edc4ceab43a37f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 8 Apr 2019 15:46:20 -0400 Subject: [PATCH 4/7] cleanup after AsSingle removal --- helper/plugin/grpc_provider.go | 18 +++++------------- helper/schema/core_schema.go | 8 ++++++++ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index 09b5f9424..37c0e41b7 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -87,17 +87,17 @@ func (s *GRPCProviderServer) getDatasourceSchemaBlockForCore(name string) *confi } func (s *GRPCProviderServer) getProviderSchemaBlockForShimming() *configschema.Block { - return schema.InternalMap(s.provider.Schema).CoreConfigSchemaForShimming() + return schema.InternalMap(s.provider.Schema).CoreConfigSchema() } func (s *GRPCProviderServer) getResourceSchemaBlockForShimming(name string) *configschema.Block { res := s.provider.ResourcesMap[name] - return res.CoreConfigSchemaForShimming() + return res.CoreConfigSchema() } func (s *GRPCProviderServer) getDatasourceSchemaBlockForShimming(name string) *configschema.Block { dat := s.provider.DataSourcesMap[name] - return dat.CoreConfigSchemaForShimming() + return dat.CoreConfigSchema() } func (s *GRPCProviderServer) PrepareProviderConfig(_ context.Context, req *proto.PrepareProviderConfig_Request) (*proto.PrepareProviderConfig_Response, error) { @@ -190,7 +190,6 @@ func (s *GRPCProviderServer) PrepareProviderConfig(_ context.Context, req *proto } config := terraform.NewResourceConfigShimmed(configVal, blockForShimming) - schema.FixupAsSingleResourceConfigIn(config, s.provider.Schema) warns, errs := s.provider.Validate(config) resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, convert.WarnsAndErrsToProto(warns, errs)) @@ -219,7 +218,6 @@ func (s *GRPCProviderServer) ValidateResourceTypeConfig(_ context.Context, req * } config := terraform.NewResourceConfigShimmed(configVal, blockForShimming) - schema.FixupAsSingleResourceConfigIn(config, s.provider.ResourcesMap[req.TypeName].Schema) warns, errs := s.provider.ValidateResource(req.TypeName, config) resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, convert.WarnsAndErrsToProto(warns, errs)) @@ -246,7 +244,6 @@ func (s *GRPCProviderServer) ValidateDataSourceConfig(_ context.Context, req *pr } config := terraform.NewResourceConfigShimmed(configVal, blockForShimming) - schema.FixupAsSingleResourceConfigIn(config, s.provider.DataSourcesMap[req.TypeName].Schema) warns, errs := s.provider.ValidateDataSource(req.TypeName, config) resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, convert.WarnsAndErrsToProto(warns, errs)) @@ -324,7 +321,7 @@ func (s *GRPCProviderServer) upgradeFlatmapState(version int, m map[string]strin // first determine if we need to call the legacy MigrateState func requiresMigrate := version < res.SchemaVersion - schemaType := res.CoreConfigSchemaForShimming().ImpliedType() + schemaType := res.CoreConfigSchema().ImpliedType() // if there are any StateUpgraders, then we need to only compare // against the first version there @@ -435,7 +432,6 @@ func (s *GRPCProviderServer) Configure(_ context.Context, req *proto.Configure_R } config := terraform.NewResourceConfigShimmed(configVal, blockForShimming) - schema.FixupAsSingleResourceConfigIn(config, s.provider.Schema) err = s.provider.Configure(config) resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) @@ -484,7 +480,6 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso // helper/schema should always copy the ID over, but do it again just to be safe newInstanceState.Attributes["id"] = newInstanceState.ID - schema.FixupAsSingleInstanceStateOut(newInstanceState, s.provider.ResourcesMap[req.TypeName]) newStateVal, err := hcl2shim.HCL2ValueFromFlatmap(newInstanceState.Attributes, blockForShimming.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) @@ -569,7 +564,6 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl // turn the proposed state into a legacy configuration cfg := terraform.NewResourceConfigShimmed(proposedNewStateVal, blockForShimming) - schema.FixupAsSingleResourceConfigIn(cfg, s.provider.ResourcesMap[req.TypeName].Schema) diff, err := s.provider.SimpleDiff(info, priorState, cfg) if err != nil { @@ -602,7 +596,7 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl } // now we need to apply the diff to the prior state, so get the planned state - plannedAttrs, err := diff.Apply(priorState.Attributes, block) + plannedAttrs, err := diff.Apply(priorState.Attributes, blockForShimming) plannedStateVal, err := hcl2shim.HCL2ValueFromFlatmap(plannedAttrs, blockForShimming.ImpliedType()) if err != nil { @@ -829,7 +823,6 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) } - schema.FixupAsSingleInstanceStateOut(newInstanceState, s.provider.ResourcesMap[req.TypeName]) newStateVal := cty.NullVal(blockForShimming.ImpliedType()) // Always return a null value for destroy. @@ -968,7 +961,6 @@ func (s *GRPCProviderServer) ReadDataSource(_ context.Context, req *proto.ReadDa } config := terraform.NewResourceConfigShimmed(configVal, blockForShimming) - schema.FixupAsSingleResourceConfigIn(config, s.provider.DataSourcesMap[req.TypeName].Schema) // we need to still build the diff separately with the Read method to match // the old behavior diff --git a/helper/schema/core_schema.go b/helper/schema/core_schema.go index 875677eb3..d563ab8ca 100644 --- a/helper/schema/core_schema.go +++ b/helper/schema/core_schema.go @@ -174,6 +174,14 @@ func (s *Schema) coreConfigSchemaBlock() *configschema.NestedBlock { // coreConfigSchemaType determines the core config schema type that corresponds // to a particular schema's type. func (s *Schema) coreConfigSchemaType() cty.Type { + if s.SkipCoreTypeCheck { + // If we're preparing a schema for Terraform Core and the schema is + // asking us to skip the Core type-check then we'll tell core that this + // attribute is dynamically-typed, so it'll just pass through anything + // and let us validate it on the plugin side. + return cty.DynamicPseudoType + } + switch s.Type { case TypeString: return cty.String From 8730d993091700706ba97000ffc7f09b48c4a82a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 8 Apr 2019 15:24:38 -0400 Subject: [PATCH 5/7] LegacyResourceSchema to remove 0.12 features This allows us to call CoreConfigSchema and return something that looks like the original schema. --- helper/schema/shims.go | 43 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/helper/schema/shims.go b/helper/schema/shims.go index d99fb3965..eccd4eb18 100644 --- a/helper/schema/shims.go +++ b/helper/schema/shims.go @@ -88,3 +88,46 @@ func JSONMapToStateValue(m map[string]interface{}, block *configschema.Block) (c func StateValueFromInstanceState(is *terraform.InstanceState, ty cty.Type) (cty.Value, error) { return is.AttrsAsObjectValue(ty) } + +// LegacyResourceSchema takes a *Resource and returns a deep copy with 0.12 specific +// features removed. This is used by the shims to get a configschema that +// directly matches the structure of the schema.Resource. +func LegacyResourceSchema(r *Resource) *Resource { + if r == nil { + return nil + } + // start with a shallow copy + newResource := new(Resource) + *newResource = *r + newResource.Schema = map[string]*Schema{} + + for k, s := range r.Schema { + newResource.Schema[k] = LegacySchema(s) + } + + return newResource +} + +// LegacySchema takes a *Schema and returns a deep copy with 0.12 specific +// features removed. This is used by the shims to get a configschema that +// directly matches the structure of the schema.Resource. +func LegacySchema(s *Schema) *Schema { + if s == nil { + return nil + } + // start with a shallow copy + newSchema := new(Schema) + *newSchema = *s + newSchema.ConfigMode = SchemaConfigModeAuto + newSchema.PromoteSingle = false + newSchema.SkipCoreTypeCheck = false + + switch e := newSchema.Elem.(type) { + case *Schema: + newSchema.Elem = LegacySchema(e) + case *Resource: + newSchema.Elem = LegacyResourceSchema(e) + } + + return newSchema +} From a3d58665ad429c6a11fa22df8be1f8da51aac55b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 8 Apr 2019 16:07:44 -0400 Subject: [PATCH 6/7] use LegacyResourceSchema rather than the previous .CoreConfigSchemaForShimming --- builtin/providers/test/diff_apply_test.go | 2 +- helper/plugin/grpc_provider.go | 13 +++++++++---- plugin/grpc_provider.go | 1 - 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/builtin/providers/test/diff_apply_test.go b/builtin/providers/test/diff_apply_test.go index 0ee5f4102..4318f0025 100644 --- a/builtin/providers/test/diff_apply_test.go +++ b/builtin/providers/test/diff_apply_test.go @@ -130,7 +130,7 @@ func TestDiffApply_set(t *testing.T) { "id": "testID", } - attrs, err := diff.Apply(priorAttrs, (&schema.Resource{Schema: resSchema}).CoreConfigSchemaForShimming()) + attrs, err := diff.Apply(priorAttrs, schema.LegacyResourceSchema(&schema.Resource{Schema: resSchema}).CoreConfigSchema()) if err != nil { t.Fatal(err) } diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index 37c0e41b7..f0e6af3e6 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -87,17 +87,22 @@ func (s *GRPCProviderServer) getDatasourceSchemaBlockForCore(name string) *confi } func (s *GRPCProviderServer) getProviderSchemaBlockForShimming() *configschema.Block { - return schema.InternalMap(s.provider.Schema).CoreConfigSchema() + newSchema := map[string]*schema.Schema{} + for attr, s := range s.provider.Schema { + newSchema[attr] = schema.LegacySchema(s) + } + + return schema.InternalMap(newSchema).CoreConfigSchema() } func (s *GRPCProviderServer) getResourceSchemaBlockForShimming(name string) *configschema.Block { res := s.provider.ResourcesMap[name] - return res.CoreConfigSchema() + return schema.LegacyResourceSchema(res).CoreConfigSchema() } func (s *GRPCProviderServer) getDatasourceSchemaBlockForShimming(name string) *configschema.Block { dat := s.provider.DataSourcesMap[name] - return dat.CoreConfigSchema() + return schema.LegacyResourceSchema(dat).CoreConfigSchema() } func (s *GRPCProviderServer) PrepareProviderConfig(_ context.Context, req *proto.PrepareProviderConfig_Request) (*proto.PrepareProviderConfig_Response, error) { @@ -321,7 +326,7 @@ func (s *GRPCProviderServer) upgradeFlatmapState(version int, m map[string]strin // first determine if we need to call the legacy MigrateState func requiresMigrate := version < res.SchemaVersion - schemaType := res.CoreConfigSchema().ImpliedType() + schemaType := schema.LegacyResourceSchema(res).CoreConfigSchema().ImpliedType() // if there are any StateUpgraders, then we need to only compare // against the first version there diff --git a/plugin/grpc_provider.go b/plugin/grpc_provider.go index 96409d712..ff1db04cd 100644 --- a/plugin/grpc_provider.go +++ b/plugin/grpc_provider.go @@ -191,7 +191,6 @@ func (p *GRPCProvider) PrepareProviderConfig(r providers.PrepareProviderConfigRe func (p *GRPCProvider) ValidateResourceTypeConfig(r providers.ValidateResourceTypeConfigRequest) (resp providers.ValidateResourceTypeConfigResponse) { log.Printf("[TRACE] GRPCProvider: ValidateResourceTypeConfig") - resourceSchema := p.getResourceSchema(r.TypeName) mp, err := msgpack.Marshal(r.Config, resourceSchema.Block.ImpliedType()) From 3ec93710fcc2e9f214f3692b94957583380acd42 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 8 Apr 2019 17:12:39 -0400 Subject: [PATCH 7/7] PromoteSingle is used in 0.11 mode --- helper/schema/shims.go | 1 - 1 file changed, 1 deletion(-) diff --git a/helper/schema/shims.go b/helper/schema/shims.go index eccd4eb18..4099d82d4 100644 --- a/helper/schema/shims.go +++ b/helper/schema/shims.go @@ -119,7 +119,6 @@ func LegacySchema(s *Schema) *Schema { newSchema := new(Schema) *newSchema = *s newSchema.ConfigMode = SchemaConfigModeAuto - newSchema.PromoteSingle = false newSchema.SkipCoreTypeCheck = false switch e := newSchema.Elem.(type) {