helper/schema: Allow ResourceDiff.ForceNew on nested fields (avoid crash)

[radeksimko]

I bumped into this when trying to resolve a bug in the K8S provider, where certain fields in the schema become immutable from a certain version of K8S.

The end-developer use-case follows:

CustomizeDiff: func(diff *schema.ResourceDiff, meta interface{}) error {
	// Mutation of PersistentVolumeSource after creation is no longer allowed in 1.9+
	// See https://github.com/kubernetes/kubernetes/blob/v1.9.3/CHANGELOG-1.9.md#storage-3
	conn := meta.(*kubernetes.Clientset)
	serverVersion, err := conn.ServerVersion()
	if err != nil {
		return err
	}

	k8sVersion, err := gversion.NewVersion(serverVersion.String())
	if err != nil {
		return err
	}

	v1_9_0, _ := gversion.NewVersion("1.9.0")
	if k8sVersion.Equal(v1_9_0) || k8sVersion.GreaterThan(v1_9_0) {
		if diff.HasChange("spec.0.persistent_volume_source") {
			keys := diff.GetChangedKeys("spec.0.persistent_volume_source")
			for _, key := range keys {
				if diff.HasChange(key) {
					err := diff.ForceNew(key)
					if err != nil {
						return err
					}
				}
			}
			return nil
		}
	}

	return nil
}

Test result before patch

 make test TEST=./helper/schema
==> Checking that code complies with gofmt requirements...
go generate ./...
2018/02/28 12:57:59 Generated command/internal_plugin_list.go
go list ./helper/schema | xargs -t -n4 go test  -timeout=60s -parallel=4
go test -timeout=60s -parallel=4 github.com/hashicorp/terraform/helper/schema
2018/02/28 12:58:04 [INFO] Resource foo has dynamic attributes
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x51 pc=0x165a3da]

goroutine 718 [running]:
testing.tRunner.func1(0xc4202b13b0)
	/usr/local/Cellar/go/1.10/libexec/src/testing/testing.go:742 +0x29d
panic(0x17c2200, 0x1d88230)
	/usr/local/Cellar/go/1.10/libexec/src/runtime/panic.go:505 +0x229
github.com/hashicorp/terraform/helper/schema.(*ResourceDiff).ForceNew(0xc420466b00, 0x18a38c0, 0x9, 0xc42046e0a0, 0xc420466b00)
	/Users/radeksimko/gopath/src/github.com/hashicorp/terraform/helper/schema/resource_diff.go:313 +0xda
github.com/hashicorp/terraform/helper/schema.TestForceNew.func1(0xc4202b13b0)
	/Users/radeksimko/gopath/src/github.com/hashicorp/terraform/helper/schema/resource_diff_test.go:769 +0x9b
testing.tRunner(0xc4202b13b0, 0xc42046aa70)
	/usr/local/Cellar/go/1.10/libexec/src/testing/testing.go:777 +0xd0
created by testing.(*T).Run
	/usr/local/Cellar/go/1.10/libexec/src/testing/testing.go:824 +0x2e0
FAIL	github.com/hashicorp/terraform/helper/schema	0.061s
make: *** [test] Error 1

[vancluever]

Hey @radeksimko, the bugfix to ForceNew looks good and makes sense. 👍

The exposure of ResourceAttrDiff via the new GetDiffAttributes function though is something that I have feedback on and I think should be possibly done another way for now, as we currently do not expose the lower level diff via ResourceDiff.

[vancluever]

I think this is just a new function on ResourceDiff and not necessarily a part of the resourceDiffer interface (which ResourceData also implements), right?

[radeksimko]

So by resourceDiffer here I meant CustomizeDiff function - I will change the wording to make it a little bit more obvious... :)

[vancluever]

This sets a precedent we haven't really implemented in ResourceDiff yet which is direct exposure of the lower-level diff. Currently ResourceDiff is mainly just a higher-level interface to the values within the diff, allowing for control of the diff for computed values by adding a new writer that allows the write of values and temporary alteration of the schema (for ForceNew operations).

I think my question in asking if such a function is necessary is what is the current issue with the functions that already exist, such as Get and GetChange? They should be able to get nested attributes just fine as they access the same readers ResourceData does.

I think maybe in the example that you are giving, you could probably just either walk the schema itself directly (by accessing it via the standard resource generator function), or hard-code the keys you want to mark as ForceNew?

[radeksimko]

hard-code the keys you want to mark as ForceNew

The schema is really deeply nested and long and it is likely to grow in the future:
https://github.com/terraform-providers/terraform-provider-kubernetes/blob/master/kubernetes/schema_volume_source.go#L14
so checking each field seems like a rather tedious approach.

maybe in the example that you are giving, you could probably just either walk the schema itself directly

That means we'd have to take the schema and try to "guess" how does it (or may) translate into keys (indexes) in the diff, which is difficult to do specifically for TypeList/TypeSet/TypeMap. It also seems like that's a logic that should be somewhere in core anyway - the provider IMO shouldn't have to traverse its own schema and know how to translate it to "diff-like format" in order to call d.Get(key) or d.GetChange(key).

However I understand what you're saying and I did have similar feelings when cobbling the PR. It does not feel right to expose the diff like this, so thanks for confirming my thoughts. 😃

I'm playing with two ideas at this point:

• func (d *ResourceDiff) ForceNewPrefix(prefix) error which would basically go through the schema and mark all nested fields (those which have the given prefix in state) as ForceNew
• func (d *ResourceDiff) GetChangedKeys(prefix) []string - here we'd be still exposing the diff, but only keys, so the bare minimum we need to (in order to call d.Get() and d.ForceNew() on them), not the whole map[string]*terraform.ResourceAttrDiff

What do you think?

[apparentlymart]

Just wanted to chime in here and agree with @vancluever's reservation... we should not expose any terraform package types in the helper/schema API because our future grpc-based protocol will not be able to marshal these.

(There is one existing violation of that rule in the schema migration mechanism which we'll be designing a special workaround for, but we should avoid introducing any new dependencies like this here.)

[vancluever]

@radeksimko I like both of those ideas, and almost think GetChangedKeys would be nice for ResourceData to have too if it weren't for the fact that the diff reader is not available in Read.

My one concern for GetChangedKeys is feature overlap with HasChange, but that's kind of a gut reaction over a logical one, especially considering you could debate that GetChange has overlap as well by the same logic. As well, I'm wondering if two versions of GetChangedKeys would be nice where you didn't have to supply the prefix and you could get the entire set of changes resource-wide, versus having to work with a possibly semantically awkward GetChangedKeys("").

But yeah, I think that's a great idea, and barring @apparentlymart saying something otherwise I don't think that exposing just the keys from the diff is risky in regards to any future changes in the API.

[radeksimko]

I'm wondering if two versions of GetChangedKeys would be nice where you didn't have to supply the prefix and you could get the entire set of changes resource-wide, versus having to work with a possibly semantically awkward GetChangedKeys("").

🤔 I can't think of a particular use case for GetChangedKeys(""). The problem I'm trying to solve would emerge when there are nested fields involved, so you would always pass a prefix, IMO.

I'm open to suggestions around names though. Should we have GetChangedKeysWithPrefix("prefix") and GetChangedKeys() - or should we have GetChangedKeysWithPrefix regardless of whether no-argument function becomes a thing? I guess it would make it easier to implement later without breaking changes.

[vancluever]

I can't think of a particular use case for GetChangedKeys(""). The problem I'm trying to solve would emerge when there are nested fields involved, so you would always pass a prefix, IMO.

Yeah, I'm not 100% sure either, but I didn't necessarily want to rule out the possibility. After giving it further thought I can't really think of one right now either.

I think maybe With is maybe a bit wordy? Maybe GetChangedKeysPrefix would be a bit more terse?

[radeksimko]

Maybe GetChangedKeysPrefix would be a bit more terse?

@vancluever Good idea. PTAL.

[apparentlymart]

As some context here, I wanted to share some info about what the workflow might look like with the protocol changes we were discussing. There's an internal design sketch on this which you both have access to, but I'm going to copy some relevant parts here for the benefit of others who might view this PR; of course the details are are, as usual, subject to change as we learn more during implementation.

The planned new interface for diffing in the wire protocol (not in the helper/schema API) has the following pseudocode-ish interface: (this is not the actual physical signature, which will instead be expressed in protocol buffers as we transition to grpc)

func PlanResourceChange(
    typeName string,
    oldState object,
    proposedNewState object,
) returns (
    newState object,
    requiresReplace []path,
    diagnostics,
)

A change compared to the current protocol is that Terraform Core has already done some of the preparation work here and provides a "proposed new state" rather than the configuration directly. This already includes the preserved values from computed fields and has already been type-checked and type-converted to conform to the resource schema.

The pseudo-type object in the above is a representation of an HCL object, and so (unlike today) we have a proper, schema-conforming, consistent data structure for oldState, proposedNewState and newState, and so it should be relatively easier to deal with attributes nested inside collections because the "flatmap" machinery will no longer be present.

I think the role of ResourceDiff in this new world is to provide an interface that reads from both oldState and proposedNewState (for d.GetChange, for example) and allows the caller to mutate a data structure that will eventually be converted into this object type to return as newState, along with the annotations in requiresReplace.

The []path type there is a "sidecar" data structure that points to attributes within the newState that should have the (forces new resource) annotation applied, and it represents a traversal through the data structure, like a more structured version of the string keys we use with functions like d.ForceNew here. So the ForceNew function in ResourceDiff in this new world would presumably add new entries to that set, after translating the given string path into a structured path.

---

With all of that said, then: I think we can support something like GetChangedKeys in the new model without too much trouble, as long as the given path refers to something that is a schema.Resource in the schema. It would traverse through both oldState and proposedNewState to get the old and new object values and then iterate over all of the attributes and compare their old and new values to produce the list.

For ForceNewPrefix, I think that wouldn't actually be required as a separate function anymore because the requiresReplace set can just include the container itself, due to the planned new diff renderer. I'd previously shared a screenshot that didn't include any (forces new resource) collections, but here's another one that shows how it might appear if you were to call ForceNew only with the collections themselves:

The # (forces new resource) annotations are added here by comparing the path of the value being rendered to the set of paths in requiresReplace, so it's valid to put any level of traversal depth into that set depending on what you're trying to communicate to the user.

(Please excuse that this screenshot is not a perfect fit for the discussion at hand; I cribbed it from my earlier prototyping results.)

[jbardin]

LGTM! I kind of like anything that adds more test cases before the "big refactor" :D

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.