Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor detailed diff v2 to short circuit on nulls and unknowns #2496

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Refactor detailed diff v2 to short circuit on nulls and unknowns #2496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
629a5ff
refactor detailed diff v2 to short circuit on nulls and unknowns
VenelinMartinov Oct 17, 2024
49ec2be
move type mismatch check and short-circuit logic
VenelinMartinov Oct 18, 2024
e13ae47
rename
VenelinMartinov Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
238 changes: 140 additions & 98 deletions pkg/tfbridge/detailed_diff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,105 @@ func sortedMergedKeys[K cmp.Ordered, V any, M ~map[K]V](a, b M) []K {
return keysSlice
}

func isTypeShapeMismatched(val resource.PropertyValue, propType shim.ValueType) bool {
if !isPresent(val) {
return false
}
switch propType {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic in isTypeShapeMismatched only works when val doesn't have any marker types, so lets assert that.

Suggested change
switch propType {
contract.Assertf(!val.IsComputed() && !val.IsSecret() && !val.IsOutput(),
"Unexpected marked value %#v", val)
switch propType {

case shim.TypeList:
return !val.IsArray()
case shim.TypeSet:
return !val.IsArray()
case shim.TypeMap:
return !val.IsObject()
default:
return false
}
}

func lookupSchemas(
path propertyPath, tfs shim.SchemaMap, ps map[string]*info.Schema,
) (shim.Schema, *info.Schema, error) {
schemaPath := PropertyPathToSchemaPath(resource.PropertyPath(path), tfs, ps)
return LookupSchemas(schemaPath, tfs, ps)
}

// walkPropertyValue walks a property value and calls the visitor function for each path in the property value.
func walkPropertyValue(
val resource.PropertyValue, path propertyPath, visitor func(propertyPath, resource.PropertyValue) bool,
) bool {
if !visitor(path, val) {
return false
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I'd like to see an assert that we are not hitting marker types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just was commenting on another PR, but this is not a complete walker on PropertyValue.

E.G.

https://github.com/pulumi/pulumi-terraform-bridge/blob/master/unstable/propertyvalue/propertyvalue.go#L42

If we had a complete walker on property value we would not need to double-test it. I know Go is about "a bit more duplication is better than a bit more sharing", but if we can reuse that makes coverage goals easier to achieve.

switch {
case val.IsArray():
for i, elVal := range val.ArrayValue() {
if !walkPropertyValue(elVal, path.Index(i), visitor) {
return false
}
}

case val.IsObject():
for k, elVal := range val.ObjectValue() {
if !walkPropertyValue(elVal, path.Subpath(string(k)), visitor) {
return false
}
}
}
return true
}

func willTriggerReplacement(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you comment on why we need willTriggerReplacement and willTriggerReplacementRecursive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code decides presentation. We already decided if the plan is replace or not. This code decides which change to blame for replacing so it's easier to see visually why a replace is happening. Is that correct VM?

path propertyPath, rootTFSchema shim.SchemaMap, rootPulumiSchema map[string]*info.Schema,
) bool {
// A change on a property might trigger a replacement if:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function seems to do what the comment says. The rule sounds a little un-intuitive but looks like you've explored this thoroughly in tests so perhaps that's that?

If a property P is forcenew, but P.a.b.c.d has changed, I am guessing that the overall plan is a replace plan, is that right? If we are displaying the detailed diff as a.b.c.d has changed we are probably within rights to blame d for replacing?

// - The property itself is marked as ForceNew
// - The direct parent property is a collection (list, set, map) and is marked as ForceNew
// See pkg/cross-tests/diff_cross_test.go
// TestAttributeCollectionForceNew, TestBlockCollectionForceNew, TestBlockCollectionElementForceNew
// for a full case study of replacements in TF
tfs, ps, err := lookupSchemas(path, rootTFSchema, rootPulumiSchema)
if err != nil {
return false
}
if isForceNew(tfs, ps) {
return true
}

if len(path) == 1 {
return false
}

parent := path[:len(path)-1]
tfs, ps, err = lookupSchemas(parent, rootTFSchema, rootPulumiSchema)
if err != nil {
return false
}
// Note this is mimicking the TF behaviour, so the effective type is not considered here.
if tfs.Type() != shim.TypeList && tfs.Type() != shim.TypeSet && tfs.Type() != shim.TypeMap {
return false
}
Comment on lines +120 to +122
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that this is (1) only effected by one level up and (2) doesn't include ForceNew on objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand these - can you please expand on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that we don't search all the way up the schema tree, not just one level up.

I think I was wrong about not including objects. tfs.Type() doesn't express objects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was wondering this too. We can probably get a away with something approximate as it's presentation-layer only but worth backlogging at least.

return isForceNew(tfs, ps)
}

func willTriggerReplacementRecursive(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name for this function needs to describe what it does, not how its' implemented.

path propertyPath, value resource.PropertyValue, tfs shim.SchemaMap, ps map[string]*info.Schema,
) bool {
replacement := false
visitor := func(subpath propertyPath, val resource.PropertyValue) bool {
if willTriggerReplacement(subpath, tfs, ps) {
replacement = true
return false
}
return true
}

walkPropertyValue(value, path, visitor)

return replacement
}

func promoteToReplace(diff *pulumirpc.PropertyDiff) *pulumirpc.PropertyDiff {
if diff == nil {
return nil
Expand Down Expand Up @@ -143,17 +242,6 @@ func (k propertyPath) IsReservedKey() bool {
return leaf == "__meta" || leaf == "__defaults"
}

func mapHasReplacements(m map[detailedDiffKey]*pulumirpc.PropertyDiff) bool {
for _, diff := range m {
if diff.GetKind() == pulumirpc.PropertyDiff_ADD_REPLACE ||
diff.GetKind() == pulumirpc.PropertyDiff_DELETE_REPLACE ||
diff.GetKind() == pulumirpc.PropertyDiff_UPDATE_REPLACE {
return true
}
}
return false
}

type detailedDiffer struct {
tfs shim.SchemaMap
ps map[string]*SchemaInfo
Expand Down Expand Up @@ -182,79 +270,21 @@ func (differ detailedDiffer) getEffectiveType(path walk.SchemaPath) shim.ValueTy
return tfs.Type()
}

func (differ detailedDiffer) lookupSchemas(path propertyPath) (shim.Schema, *info.Schema, error) {
schemaPath := PropertyPathToSchemaPath(resource.PropertyPath(path), differ.tfs, differ.ps)
return LookupSchemas(schemaPath, differ.tfs, differ.ps)
}

func (differ detailedDiffer) isForceNew(pair propertyPath) bool {
// A change on a property might trigger a replacement if:
// - The property itself is marked as ForceNew
// - The direct parent property is a collection (list, set, map) and is marked as ForceNew
// See pkg/cross-tests/diff_cross_test.go
// TestAttributeCollectionForceNew, TestBlockCollectionForceNew, TestBlockCollectionElementForceNew
// for a full case study of replacements in TF
tfs, ps, err := differ.lookupSchemas(pair)
if err != nil {
return false
}
if isForceNew(tfs, ps) {
return true
}

if len(pair) == 1 {
return false
}

parent := pair[:len(pair)-1]
tfs, ps, err = differ.lookupSchemas(parent)
if err != nil {
return false
}
// Note this is mimicking the TF behaviour, so the effective type is not considered here.
if tfs.Type() != shim.TypeList && tfs.Type() != shim.TypeSet && tfs.Type() != shim.TypeMap {
return false
}
return isForceNew(tfs, ps)
}

// We do not short-circuit detailed diffs when comparing non-nil properties against nil ones. The reason for that is
// that a replace might be triggered by a ForceNew inside a nested property of a non-ForceNew property. We instead
// always walk the full tree even when comparing against a nil property. We then later do a simplification step for
// the detailed diff in simplifyDiff in order to reduce the diff to what the user expects to see.
// See [pulumi/pulumi-terraform-bridge#2405] for more details.
func (differ detailedDiffer) simplifyDiff(
diff map[detailedDiffKey]*pulumirpc.PropertyDiff, path propertyPath, old, new resource.PropertyValue,
) (map[detailedDiffKey]*pulumirpc.PropertyDiff, bool) {
baseDiff := makeBaseDiff(old, new)
if baseDiff == undecidedDiff {
return nil, false
}
propDiff := baseDiff.ToPropertyDiff()
if propDiff == nil {
return nil, true
}
if differ.isForceNew(path) || mapHasReplacements(diff) {
propDiff = promoteToReplace(propDiff)
}
return map[detailedDiffKey]*pulumirpc.PropertyDiff{path.Key(): propDiff}, true
}

// makePlainPropDiff is used for plain properties and ones with an unknown schema.
// It does not access the TF schema, so it does not know about the type of the property.
func (differ detailedDiffer) makePlainPropDiff(
path propertyPath, old, new resource.PropertyValue,
) map[detailedDiffKey]*pulumirpc.PropertyDiff {
baseDiff := makeBaseDiff(old, new)
isForceNew := differ.isForceNew(path)
isReplacement := willTriggerReplacement(path, differ.tfs, differ.ps)
var propDiff *pulumirpc.PropertyDiff
if baseDiff != undecidedDiff {
propDiff = baseDiff.ToPropertyDiff()
} else if !old.DeepEquals(new) {
propDiff = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE}
}

if isForceNew {
if isReplacement {
propDiff = promoteToReplace(propDiff)
}

Expand All @@ -264,13 +294,48 @@ func (differ detailedDiffer) makePlainPropDiff(
return nil
}

// makeShortCircuitDiff is used for properties that are nil or computed in either the old or new state.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is .. tricky.. If a property is Computed or unknown, we do not know if it will trigger a replacement or not, until we know it. It might trigger a replacement, or it might not. Any implementation here takes a guess and we need to probably call this out in a comment that we are guessing. I would look up current Pulumi behavior for this case and copy how we're guessing to continue in the tradition we have.

// It makes sure to check recursively if the property will trigger a replacement.
func (differ detailedDiffer) makeShortCircuitDiff(
path propertyPath, old, new resource.PropertyValue,
) map[detailedDiffKey]*pulumirpc.PropertyDiff {
contract.Assertf(old.IsNull() || new.IsNull() || new.IsComputed(),
"short-circuit diff should only be used for nil properties")
if old.IsNull() && new.IsNull() {
return nil
}

baseDiff := makeBaseDiff(old, new)
contract.Assertf(baseDiff != undecidedDiff, "short-circuit diff could not determine diff kind")

propDiff := baseDiff.ToPropertyDiff()
if new.IsComputed() && willTriggerReplacement(path, differ.tfs, differ.ps) {
propDiff = promoteToReplace(propDiff)
} else if !new.IsNull() && !new.IsComputed() && willTriggerReplacementRecursive(path, new, differ.tfs, differ.ps) {
propDiff = promoteToReplace(propDiff)
} else if !old.IsNull() && willTriggerReplacementRecursive(path, old, differ.tfs, differ.ps) {
propDiff = promoteToReplace(propDiff)
}

return map[detailedDiffKey]*pulumirpc.PropertyDiff{path.Key(): propDiff}
}

func (differ detailedDiffer) makePropDiff(
path propertyPath, old, new resource.PropertyValue,
) map[detailedDiffKey]*pulumirpc.PropertyDiff {
if path.IsReservedKey() {
return nil
}
propType := differ.getEffectiveType(differ.propertyPathToSchemaPath(path))
if !isPresent(old) || isTypeShapeMismatched(old, propType) {
old = resource.NewNullProperty()
}
if !isPresent(new) || isTypeShapeMismatched(new, propType) && !new.IsComputed() {
new = resource.NewNullProperty()
}
if old.IsNull() || new.IsNull() || new.IsComputed() {
return differ.makeShortCircuitDiff(path, old, new)
}

switch propType {
case shim.TypeList:
Expand All @@ -290,14 +355,8 @@ func (differ detailedDiffer) makeListDiff(
path propertyPath, old, new resource.PropertyValue,
) map[detailedDiffKey]*pulumirpc.PropertyDiff {
diff := make(map[detailedDiffKey]*pulumirpc.PropertyDiff)
oldList := []resource.PropertyValue{}
newList := []resource.PropertyValue{}
if isPresent(old) && old.IsArray() {
oldList = old.ArrayValue()
}
if isPresent(new) && new.IsArray() {
newList = new.ArrayValue()
}
oldList := old.ArrayValue()
newList := new.ArrayValue()

// naive diffing of lists
// TODO[pulumi/pulumi-terraform-bridge#2295]: implement a more sophisticated diffing algorithm
Expand All @@ -318,27 +377,15 @@ func (differ detailedDiffer) makeListDiff(
}
}

simplerDiff, isSimplified := differ.simplifyDiff(diff, path, old, new)
if isSimplified {
return simplerDiff
}

return diff
}

func (differ detailedDiffer) makeMapDiff(
path propertyPath, old, new resource.PropertyValue,
) map[detailedDiffKey]*pulumirpc.PropertyDiff {
oldMap := old.ObjectValue()
newMap := new.ObjectValue()
Comment on lines +386 to +387
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so I understand, this is ok because of isTypeShapeMismatched?

diff := make(map[detailedDiffKey]*pulumirpc.PropertyDiff)
oldMap := resource.PropertyMap{}
newMap := resource.PropertyMap{}
if isPresent(old) && old.IsObject() {
oldMap = old.ObjectValue()
}
if isPresent(new) && new.IsObject() {
newMap = new.ObjectValue()
}

for _, k := range sortedMergedKeys(oldMap, newMap) {
subindex := path.Subpath(string(k))
oldVal := oldMap[k]
Expand All @@ -351,11 +398,6 @@ func (differ detailedDiffer) makeMapDiff(
}
}

simplerDiff, isSimplified := differ.simplifyDiff(diff, path, old, new)
if isSimplified {
return simplerDiff
}

return diff
}

Expand Down
22 changes: 8 additions & 14 deletions pkg/tfbridge/detailed_diff_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,9 @@ func TestSchemaLookupMaxItemsOnePlain(t *testing.T) {
},
}

differ := detailedDiffer{
tfs: shimv2.NewSchemaMap(sdkv2Schema),
}
tfs := shimv2.NewSchemaMap(sdkv2Schema)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to tests look very unconvincing 🙏

You're going for no change in behavior in this PR?

Can you point me to a test case that illustrates this "short-circuit" diff so I can build up some intuition? Perhaps this all is under tests already. Thank you!

Do not want to block on this but would like to understand better.


sch, _, err := differ.lookupSchemas(newPropertyPath("string_prop"))
sch, _, err := lookupSchemas(newPropertyPath("string_prop"), tfs, nil)
require.NoError(t, err)
require.NotNil(t, sch)
require.Equal(t, sch.Type(), shim.TypeList)
Expand All @@ -71,16 +69,14 @@ func TestSchemaLookupMaxItemsOne(t *testing.T) {
},
}

differ := detailedDiffer{
tfs: shimv2.NewSchemaMap(res.Schema),
}
tfs := shimv2.NewSchemaMap(res.Schema)

sch, _, err := differ.lookupSchemas(newPropertyPath("foo"))
sch, _, err := lookupSchemas(newPropertyPath("foo"), tfs, nil)
require.NoError(t, err)
require.NotNil(t, sch)
require.Equal(t, sch.Type(), shim.TypeList)

sch, _, err = differ.lookupSchemas(newPropertyPath("foo").Subpath("bar"))
sch, _, err = lookupSchemas(newPropertyPath("foo").Subpath("bar"), tfs, nil)
require.NoError(t, err)
require.NotNil(t, sch)
require.Equal(t, sch.Type(), shim.TypeString)
Expand All @@ -100,16 +96,14 @@ func TestSchemaLookupMap(t *testing.T) {
},
}

differ := detailedDiffer{
tfs: shimv2.NewSchemaMap(res.Schema),
}
tfs := shimv2.NewSchemaMap(res.Schema)

sch, _, err := differ.lookupSchemas(newPropertyPath("foo"))
sch, _, err := lookupSchemas(newPropertyPath("foo"), tfs, nil)
require.NoError(t, err)
require.NotNil(t, sch)
require.Equal(t, sch.Type(), shim.TypeMap)

sch, _, err = differ.lookupSchemas(propertyPath{"foo", "bar"})
sch, _, err = lookupSchemas(newPropertyPath("foo").Subpath("bar"), tfs, nil)
require.NoError(t, err)
require.NotNil(t, sch)
require.Equal(t, sch.Type(), shim.TypeString)
Expand Down
Loading