From 457ffdc22209e8af584314edd350e8abc0a0b324 Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Thu, 9 May 2024 13:24:50 -0400 Subject: [PATCH 1/7] Initial pass --- pkg/tfbridge/deconflict.go | 112 ++++++++++++++++++++++++++++++++ pkg/tfbridge/deconflict_test.go | 28 ++++++++ 2 files changed, 140 insertions(+) create mode 100644 pkg/tfbridge/deconflict.go create mode 100644 pkg/tfbridge/deconflict_test.go diff --git a/pkg/tfbridge/deconflict.go b/pkg/tfbridge/deconflict.go new file mode 100644 index 000000000..29ecb039a --- /dev/null +++ b/pkg/tfbridge/deconflict.go @@ -0,0 +1,112 @@ +// Copyright 2016-2024, Pulumi Corporation. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package tfbridge + +import ( + "context" + "fmt" + "strconv" + "strings" + + "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge/info" + shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim" + "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/walk" + "github.com/pulumi/pulumi-terraform-bridge/v3/unstable/propertyvalue" + "github.com/pulumi/pulumi/sdk/v3/go/common/resource" + "github.com/pulumi/pulumi/sdk/v3/go/common/util/contract" +) + +// Transforms inputs to enforce input bag conformance with ConflictsWith constraints. +// +// This arbitrarily drops properties until none are left in conflict. +// +// Why is this necessary: Read in Pulumi is expected to produce an input bag that is going to subsequently pass Check, +// but TF providers are not always well-behaved in this regard. +func deconflict( + ctx context.Context, + schemaMap shim.SchemaMap, + schemaInfos map[string]*info.Schema, + inputs resource.PropertyMap, +) resource.PropertyMap { + cm := newConflictsMap(schemaMap) + visitedPaths := map[string]struct{}{} + visit := func(pp resource.PropertyPath, pv resource.PropertyValue) (resource.PropertyValue, error) { + sp := PropertyPathToSchemaPath(pp, schemaMap, schemaInfos) + conflict := false + var conflictAt walk.SchemaPath + for _, cp := range cm.ConflictingPaths(sp) { + if _, ok := visitedPaths[cp.MustEncodeSchemaPath()]; ok { + conflict = true + conflictAt = cp + break + } + } + if conflict { + msg := fmt.Sprintf("Dropping property at %q to respect ConflictsWith constraint from %q", + pp.String(), conflictAt.MustEncodeSchemaPath()) + GetLogger(ctx).Debug(msg) + return resource.NewNullProperty(), nil + } + visitedPaths[sp.MustEncodeSchemaPath()] = struct{}{} + return pv, nil + } + + obj := resource.NewObjectProperty(inputs) + pv, err := propertyvalue.TransformPropertyValue(resource.PropertyPath{}, visit, obj) + contract.AssertNoErrorf(err, "deconflict transformation is never expected to fail") + contract.Assertf(pv.IsObject(), "deconflict transformation is not expected to change objects to something else") + return pv.ObjectValue() +} + +type conflictsMap struct { + conflicts map[string][]walk.SchemaPath +} + +func (cm *conflictsMap) ConflictingPaths(atPath walk.SchemaPath) []walk.SchemaPath { + return cm.conflicts[atPath.MustEncodeSchemaPath()] +} + +func (cm *conflictsMap) AddConflict(atPath walk.SchemaPath, conflictingPaths []walk.SchemaPath) { + cm.conflicts[atPath.MustEncodeSchemaPath()] = conflictingPaths +} + +func newConflictsMap(schemaMap shim.SchemaMap) *conflictsMap { + cm := &conflictsMap{conflicts: map[string][]walk.SchemaPath{}} + walk.VisitSchemaMap(schemaMap, func(sp walk.SchemaPath, s shim.Schema) { + if len(s.ConflictsWith()) > 0 { + conflictingPaths := []walk.SchemaPath{} + for _, p := range s.ConflictsWith() { + conflictingPaths = append(conflictingPaths, parseConflictsWith(p)) + } + cm.AddConflict(sp, conflictingPaths) + } + }) + return cm +} + +func parseConflictsWith(s string) walk.SchemaPath { + parts := strings.Split(s, ".") + result := walk.NewSchemaPath() + for _, p := range parts { + _, strconvErr := strconv.Atoi(p) + isNum := strconvErr == nil + if isNum { + result = result.Element() + } else { + result = result.GetAttr(p) + } + } + return result +} diff --git a/pkg/tfbridge/deconflict_test.go b/pkg/tfbridge/deconflict_test.go new file mode 100644 index 000000000..76a389573 --- /dev/null +++ b/pkg/tfbridge/deconflict_test.go @@ -0,0 +1,28 @@ +// Copyright 2016-2024, Pulumi Corporation. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package tfbridge + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestParseConflictsWith(t *testing.T) { + cw := "capacity_reservation_specification.0.capacity_reservation_target.0.capacity_reservation_id" + actual := parseConflictsWith(cw) + expect := "capacity_reservation_specification.$.capacity_reservation_target.$.capacity_reservation_id" + require.Equal(t, expect, actual.MustEncodeSchemaPath()) +} From abb63fce4797d1381ec5b6975dd6c5fe46743fae Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Thu, 9 May 2024 13:47:02 -0400 Subject: [PATCH 2/7] Install deconflict pass into Read --- pkg/tfbridge/provider.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/tfbridge/provider.go b/pkg/tfbridge/provider.go index abc17c5d8..ae6da3787 100644 --- a/pkg/tfbridge/provider.go +++ b/pkg/tfbridge/provider.go @@ -1239,7 +1239,10 @@ func (p *Provider) Read(ctx context.Context, req *pulumirpc.ReadRequest) (*pulum if err != nil { return nil, err } - minputs, err := plugin.MarshalProperties(inputs, plugin.MarshalOptions{ + + cleanInputs := deconflict(ctx, res.TF.Schema(), res.Schema.Fields, inputs) + + minputs, err := plugin.MarshalProperties(cleanInputs, plugin.MarshalOptions{ Label: label + ".inputs", KeepSecrets: p.supportsSecrets, }) From 8d25602bddba3d0de28ffc0ec8824bf5229a0656 Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Thu, 9 May 2024 15:28:28 -0400 Subject: [PATCH 3/7] Tests --- pkg/tfbridge/deconflict_test.go | 54 +++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/pkg/tfbridge/deconflict_test.go b/pkg/tfbridge/deconflict_test.go index 76a389573..44422444f 100644 --- a/pkg/tfbridge/deconflict_test.go +++ b/pkg/tfbridge/deconflict_test.go @@ -15,11 +15,65 @@ package tfbridge import ( + "context" + "fmt" "testing" + shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim" + "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/schema" + "github.com/pulumi/pulumi-terraform-bridge/v3/unstable/logging" + "github.com/pulumi/pulumi/sdk/v3/go/common/resource" "github.com/stretchr/testify/require" ) +func TestDeconflict(t *testing.T) { + ctx := context.Background() + + ctx = logging.InitLogging(ctx, logging.LogOptions{}) + schema := &schema.SchemaMap{ + "availability_zone": (&schema.Schema{ + Type: shim.TypeString, + Optional: true, + Computed: true, + ForceNew: true, + ConflictsWith: []string{"availability_zone_id"}, + }).Shim(), + "availability_zone_id": (&schema.Schema{ + Type: shim.TypeString, + Optional: true, + Computed: true, + ForceNew: true, + ConflictsWith: []string{"availability_zone"}, + }).Shim(), + } + + type testCase struct { + inputs resource.PropertyMap + expected resource.PropertyMap + } + + testCases := []testCase{ + { + inputs: resource.PropertyMap{ + "availabilityZone": resource.NewStringProperty("us-east-1c"), + "availabilityZoneId": resource.NewStringProperty("use1-az1"), + }, + expected: resource.PropertyMap{ + "availabilityZone": resource.NewStringProperty("us-east-1c"), + "availabilityZoneId": resource.NewNullProperty(), + }, + }, + } + + for i, tc := range testCases { + tc := tc + t.Run(fmt.Sprintf("case-%d", i), func(t *testing.T) { + actual := deconflict(ctx, schema, nil, tc.inputs) + require.Equal(t, tc.expected, actual) + }) + } +} + func TestParseConflictsWith(t *testing.T) { cw := "capacity_reservation_specification.0.capacity_reservation_target.0.capacity_reservation_id" actual := parseConflictsWith(cw) From bfc002436a03229cf13a54e3758e4b3faf9e7a24 Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Thu, 9 May 2024 16:50:06 -0400 Subject: [PATCH 4/7] More test cases --- pkg/tfbridge/deconflict_test.go | 99 +++++++++++++++++++++++++-------- 1 file changed, 77 insertions(+), 22 deletions(-) diff --git a/pkg/tfbridge/deconflict_test.go b/pkg/tfbridge/deconflict_test.go index 44422444f..4b866bcdd 100644 --- a/pkg/tfbridge/deconflict_test.go +++ b/pkg/tfbridge/deconflict_test.go @@ -16,7 +16,6 @@ package tfbridge import ( "context" - "fmt" "testing" shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim" @@ -30,30 +29,60 @@ func TestDeconflict(t *testing.T) { ctx := context.Background() ctx = logging.InitLogging(ctx, logging.LogOptions{}) - schema := &schema.SchemaMap{ - "availability_zone": (&schema.Schema{ - Type: shim.TypeString, - Optional: true, - Computed: true, - ForceNew: true, - ConflictsWith: []string{"availability_zone_id"}, - }).Shim(), - "availability_zone_id": (&schema.Schema{ - Type: shim.TypeString, - Optional: true, - Computed: true, - ForceNew: true, - ConflictsWith: []string{"availability_zone"}, - }).Shim(), - } type testCase struct { - inputs resource.PropertyMap - expected resource.PropertyMap + name string + schemaMap shim.SchemaMap + inputs resource.PropertyMap + expected resource.PropertyMap } testCases := []testCase{ { + // The base case is not modifying inputs when there is no conflict. + name: "aws-subnet-no-conflict", + schemaMap: &schema.SchemaMap{ + "availability_zone": (&schema.Schema{ + Type: shim.TypeString, + Optional: true, + Computed: true, + ForceNew: true, + ConflictsWith: []string{"availability_zone_id"}, + }).Shim(), + "availability_zone_id": (&schema.Schema{ + Type: shim.TypeString, + Optional: true, + Computed: true, + ForceNew: true, + ConflictsWith: []string{"availability_zone"}, + }).Shim(), + }, + inputs: resource.PropertyMap{ + "availabilityZone": resource.NewStringProperty("us-east-1c"), + }, + expected: resource.PropertyMap{ + "availabilityZone": resource.NewStringProperty("us-east-1c"), + }, + }, + { + // Typical example of ConflictsWith, highly voted AWS resource excerpt here. + name: "aws-subnet", + schemaMap: &schema.SchemaMap{ + "availability_zone": (&schema.Schema{ + Type: shim.TypeString, + Optional: true, + Computed: true, + ForceNew: true, + ConflictsWith: []string{"availability_zone_id"}, + }).Shim(), + "availability_zone_id": (&schema.Schema{ + Type: shim.TypeString, + Optional: true, + Computed: true, + ForceNew: true, + ConflictsWith: []string{"availability_zone"}, + }).Shim(), + }, inputs: resource.PropertyMap{ "availabilityZone": resource.NewStringProperty("us-east-1c"), "availabilityZoneId": resource.NewStringProperty("use1-az1"), @@ -63,12 +92,38 @@ func TestDeconflict(t *testing.T) { "availabilityZoneId": resource.NewNullProperty(), }, }, + { + // Usually ConflictsWith are bi-directional but this AWS resource has it one-directional. + name: "aws-ipam-pool-cidr", + schemaMap: &schema.SchemaMap{ + "cidr": (&schema.Schema{ + Type: shim.TypeString, + Optional: true, + ForceNew: true, + Computed: true, + }).Shim(), + "netmask_length": (&schema.Schema{ + Type: shim.TypeInt, + Optional: true, + ForceNew: true, + ConflictsWith: []string{"cidr"}, + }).Shim(), + }, + inputs: resource.PropertyMap{ + "cidr": resource.NewStringProperty("192.0.2.0/24"), + "netmaskLength": resource.NewNumberProperty(24), + }, + expected: resource.PropertyMap{ + "cidr": resource.NewStringProperty("192.0.2.0/24"), + "netmaskLength": resource.NewNullProperty(), + }, + }, } - for i, tc := range testCases { + for _, tc := range testCases { tc := tc - t.Run(fmt.Sprintf("case-%d", i), func(t *testing.T) { - actual := deconflict(ctx, schema, nil, tc.inputs) + t.Run(tc.name, func(t *testing.T) { + actual := deconflict(ctx, tc.schemaMap, nil, tc.inputs) require.Equal(t, tc.expected, actual) }) } From 54f85d117d99fe7327e4d0b94d9c25f7cbb04a72 Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Thu, 9 May 2024 17:01:20 -0400 Subject: [PATCH 5/7] More tests --- pkg/tfbridge/deconflict_test.go | 89 +++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/pkg/tfbridge/deconflict_test.go b/pkg/tfbridge/deconflict_test.go index 4b866bcdd..8e5c39347 100644 --- a/pkg/tfbridge/deconflict_test.go +++ b/pkg/tfbridge/deconflict_test.go @@ -120,6 +120,95 @@ func TestDeconflict(t *testing.T) { }, } + // aws_autoscaling_group has three-way partial triangle of ConflictsWith. + asgSchema := &schema.SchemaMap{ + "load_balancers": (&schema.Schema{ + Type: shim.TypeSet, + Optional: true, + Computed: true, + Elem: (&schema.Schema{Type: shim.TypeString}).Shim(), + ConflictsWith: []string{"traffic_source"}, + }).Shim(), + "target_group_arns": (&schema.Schema{ + Type: shim.TypeSet, + Optional: true, + Computed: true, + Elem: (&schema.Schema{Type: shim.TypeString}).Shim(), + ConflictsWith: []string{"traffic_source"}, + }).Shim(), + "traffic_source": (&schema.Schema{ + Type: shim.TypeSet, + Optional: true, + Computed: true, + Elem: (&schema.Resource{ + Schema: &schema.SchemaMap{ + "identifier": (&schema.Schema{ + Type: shim.TypeString, + Required: true, + }).Shim(), + }, + }).Shim(), + ConflictsWith: []string{"load_balancers", "target_group_arns"}, + }).Shim(), + } + + testCases = append(testCases, []testCase{ + { + name: "aws-autoscaling-group-keep", + schemaMap: asgSchema, + inputs: resource.PropertyMap{ + "loadBalancers": resource.NewArrayProperty([]resource.PropertyValue{ + resource.NewStringProperty("lb1"), + resource.NewStringProperty("lb2"), + }), + "targetGroupArns": resource.NewArrayProperty([]resource.PropertyValue{ + resource.NewStringProperty("arn1"), + resource.NewStringProperty("arn2"), + }), + }, + expected: resource.PropertyMap{ + "loadBalancers": resource.NewArrayProperty([]resource.PropertyValue{ + resource.NewStringProperty("lb1"), + resource.NewStringProperty("lb2"), + }), + "targetGroupArns": resource.NewArrayProperty([]resource.PropertyValue{ + resource.NewStringProperty("arn1"), + resource.NewStringProperty("arn2"), + }), + }, + }, + { + name: "aws-autoscaling-group-drop-traffic-sources", + schemaMap: asgSchema, + inputs: resource.PropertyMap{ + "loadBalancers": resource.NewArrayProperty([]resource.PropertyValue{ + resource.NewStringProperty("lb1"), + resource.NewStringProperty("lb2"), + }), + "targetGroupArns": resource.NewArrayProperty([]resource.PropertyValue{ + resource.NewStringProperty("arn1"), + resource.NewStringProperty("arn2"), + }), + "trafficSources": resource.NewArrayProperty([]resource.PropertyValue{ + resource.NewObjectProperty(resource.PropertyMap{ + "identifier": resource.NewStringProperty("id1"), + }), + }), + }, + expected: resource.PropertyMap{ + "loadBalancers": resource.NewArrayProperty([]resource.PropertyValue{ + resource.NewStringProperty("lb1"), + resource.NewStringProperty("lb2"), + }), + "targetGroupArns": resource.NewArrayProperty([]resource.PropertyValue{ + resource.NewStringProperty("arn1"), + resource.NewStringProperty("arn2"), + }), + "trafficSources": resource.NewNullProperty(), + }, + }, + }...) + for _, tc := range testCases { tc := tc t.Run(tc.name, func(t *testing.T) { From 60b82e5d3fb13e4e12d7b98274ba6f3e81580691 Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Thu, 9 May 2024 17:07:31 -0400 Subject: [PATCH 6/7] PR feedback --- pkg/tfbridge/deconflict.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/tfbridge/deconflict.go b/pkg/tfbridge/deconflict.go index 29ecb039a..5ebcdc1ce 100644 --- a/pkg/tfbridge/deconflict.go +++ b/pkg/tfbridge/deconflict.go @@ -34,6 +34,12 @@ import ( // // Why is this necessary: Read in Pulumi is expected to produce an input bag that is going to subsequently pass Check, // but TF providers are not always well-behaved in this regard. +// +// TODO[pulumi/pulumi-terraform-bridge#1949] handle too-many ExactlyOneOf values specified violations similarly. +// +// There may be cases where the dropout strategy is going to generate new issues with ExactlyOneOf or RequiredWith +// constraints. Before a SAT solver is brought to bear at this problem to solve it in full generality, it is good to +// collect some evidence it happens in practice. func deconflict( ctx context.Context, schemaMap shim.SchemaMap, From 5ce1eda1c277d88cb887c4e6fd3f778d0e49b9a0 Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Thu, 9 May 2024 17:26:35 -0400 Subject: [PATCH 7/7] Determinism --- pkg/tfbridge/deconflict.go | 30 +++++++++++++++++++++++++++--- pkg/tfbridge/deconflict_test.go | 7 ++----- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/pkg/tfbridge/deconflict.go b/pkg/tfbridge/deconflict.go index 5ebcdc1ce..e111e974e 100644 --- a/pkg/tfbridge/deconflict.go +++ b/pkg/tfbridge/deconflict.go @@ -48,7 +48,8 @@ func deconflict( ) resource.PropertyMap { cm := newConflictsMap(schemaMap) visitedPaths := map[string]struct{}{} - visit := func(pp resource.PropertyPath, pv resource.PropertyValue) (resource.PropertyValue, error) { + + hasConflict := func(pp resource.PropertyPath) bool { sp := PropertyPathToSchemaPath(pp, schemaMap, schemaInfos) conflict := false var conflictAt walk.SchemaPath @@ -63,10 +64,33 @@ func deconflict( msg := fmt.Sprintf("Dropping property at %q to respect ConflictsWith constraint from %q", pp.String(), conflictAt.MustEncodeSchemaPath()) GetLogger(ctx).Debug(msg) - return resource.NewNullProperty(), nil + return true } visitedPaths[sp.MustEncodeSchemaPath()] = struct{}{} - return pv, nil + return false + } + + visitObject := func(pp resource.PropertyPath, pm resource.PropertyMap) (resource.PropertyMap, error) { + result := pm.Copy() + // Default TransformPropertyValue does not sort on keys; would like to avoid non-determinism here. + for _, key := range pm.StableKeys() { + subPath := append(pp, string(key)) + if hasConflict(subPath) { + delete(result, key) + } + } + return result, nil + } + + visit := func(pp resource.PropertyPath, pv resource.PropertyValue) (resource.PropertyValue, error) { + if !pv.IsObject() { + return pv, nil + } + pm, err := visitObject(pp, pv.ObjectValue()) + if err != err { + return resource.NewNullProperty(), err + } + return resource.NewObjectProperty(pm), nil } obj := resource.NewObjectProperty(inputs) diff --git a/pkg/tfbridge/deconflict_test.go b/pkg/tfbridge/deconflict_test.go index 8e5c39347..8a2b75596 100644 --- a/pkg/tfbridge/deconflict_test.go +++ b/pkg/tfbridge/deconflict_test.go @@ -88,8 +88,7 @@ func TestDeconflict(t *testing.T) { "availabilityZoneId": resource.NewStringProperty("use1-az1"), }, expected: resource.PropertyMap{ - "availabilityZone": resource.NewStringProperty("us-east-1c"), - "availabilityZoneId": resource.NewNullProperty(), + "availabilityZone": resource.NewStringProperty("us-east-1c"), }, }, { @@ -114,8 +113,7 @@ func TestDeconflict(t *testing.T) { "netmaskLength": resource.NewNumberProperty(24), }, expected: resource.PropertyMap{ - "cidr": resource.NewStringProperty("192.0.2.0/24"), - "netmaskLength": resource.NewNullProperty(), + "cidr": resource.NewStringProperty("192.0.2.0/24"), }, }, } @@ -204,7 +202,6 @@ func TestDeconflict(t *testing.T) { resource.NewStringProperty("arn1"), resource.NewStringProperty("arn2"), }), - "trafficSources": resource.NewNullProperty(), }, }, }...)