Non-determinism when value removed from configuration

[jefferai]

This obsoletes #946.

It is possible for Terraform to get into two different states given the same configuration. Here's how it can be reproduced.

1. Create a configuration file that does not set an Optional but non-Computed field.
2. Apply the configuration. Note that the tfstate file does not show an entry for the field. At this time, calling GetOk will result in ok = false.
3. Add a value for the field to the configuration.
4. Apply the configuration. Note that the tfstate file shows an entry for the field matching the configuration. At this time, calling GetOk will result in ok = true.
5. Remove the value for the field from the configuration.
6. Apply the configuration. Note that the tfstate file shows an entry for the field of the default value. At this time, calling GetOk will result in ok = true.

Given 2 and 6, Terraform is in two different states given the same resource configuration. As Terraform is supposed to be idempotent, this is a bug. Code relying on GetOk to return an expected result will experience different answers based on the history of the resource rather than the actual configuration file.

As far as I can tell, the bug stems from the fact that Terraform handles removal of values from configuration by setting them to the default value for the type because there is no notion of removal of data from ResourceData.

One way to fix this would be to treat removal of a configuration field as a setting to the default value for that field for the purposes of calculating diffs and taking action; after action has been taken, those values should not be written to the tfstate file.

Another way to fix this might be to define an undefined type for the state file; when this is the state of the field, Exists (and thus GetOk) return false. This preserves the notion that the value was, at some point, set, but is not currently. In this solution, for consistency, all non-computed fields not currently defined in configuration should be defined in the tfstate file with the undefined type.

@sparkprime as he's interested.

[sparkprime]

Just to be clear, the tfstate file at 6 ought to be the same as the one at 2 (i.e. field absent as it is unset). This absentness of the field is information that is lost during the diff / plan / update steps.

[jefferai]

FWIW, technically this would be non-determinism rather than non-idempotency. #952 is a true idempotency issue. :-) I'm not sure Terraform claims to be deterministic, but this sure seems like something that should be fixed regardless.

[sparkprime]

Let C (config) be the content of .tf and S (state) be the content of .tfstate, for a particular resource.

After application of C, if the schema of C contains no computed attributes, then it should always be the case that S = C. At the moment, this is only the case if one considers the absence of an attribute value to be equivalent to an attribute holding the default value for that type. However it is easy to imagine cases where the provider treats "absence of an input" and "zero-d input" differently, so this equivalence is dangerous. Without that equivalence, C can yield either S or S' depending on the previous value of C.

I would characterise it as imperative leaking -- S is really a function of the history of C (C, C', C'', etc.), instead of just a function of C (as is necessary to be declarative).

[jefferai]

The API provides GetOk so that you can determine whether the first value coming from the GetOk function is the default or actually came from the configuration. Due to the bug as described above, the second value is not actually telling you whether the value has been set by configuration; it's instead telling you whether a value was ever set in the configuration. One of the problems here is that there can be a difference in some upstream APIs between, for instance, a set value that empty vs. a value that is not set, and currently it may be impossible to properly instrument such an API. So it may not even be the provider that is treating things differently, but rather the APIs that it's making use of.

[jefferai]

I believe this is fixed in #993 but would appreciate confirmation.

[phinze]

Yep the ambiguity should be taken care of with the new GetOk semantics.

The API provides GetOk so that you can determine whether the first value coming from the GetOk function is the default or actually came from the configuration.

This is no longer accurate. We called out in #993 that TF currently has no way of distinguishing between "set and zero" and "not set and zero". GetOk is simply a convenient way of checking that a given field "exists and is non-zero".

If the behavior after #993 looks sane to you, @jefferai, I'm 👍 to call this closed.

[jefferai]

I do think that there are APIs that providers might need to work with that are tri-state, where a value that exists but is zero is different than a value not existing. For example, setting a domain name: not including a domain name parameter may be a clue to the downstream provider to use a default domain (e.g. "local"), whereas including a domain name parameter that is empty literally means to have no set domain name for the machine (which is a valid thing to do). As a provider, I don't see a way to properly support this.

I think that this issue is literally solved by that commit, in that behavior is now deterministic, but it doesn't solve the larger problem of GetOk having an identity crisis: despite the Ok semantics, it doesn't actually behave the same way that, for instance, a Go map does, where the Ok parameter truly tells you if the value existed in the map. And I do think that there is value in having GetOk truly tell you whether a value exists in the configuration.

Perhaps that request should be moved to a separate issue, but I'd prefer that it not be dropped altogether.

[phinze]

Breaking down the tri-states from your example:

1. IF user specifies non-empty domain name, THEN domain should be set to specified name
2. IF user omits domain name from config THEN domain should be set to default value
3. IF user specifies empty domain name, THEN domain name should be unset

And you're saying it's not clear how to distinguish between the conditions of (2) and (3) from a provider.

If you specify a Default in the Schema, you get the behavior for free; the default value will be injected by the time the provider sees it. But you're talking about a scenario where the upstream API injects the default based on an omission of the parameter from the API (perhaps by dynamically generating a value).

(Let me know if any of the above re-statement of the scenario in question is inaccurate.)

In this case, you could accomplish this from a provider by setting a schema default to a sentinel value. So your schema definition would look like:

"domain_name": &schema.Schema{
  Type:     schema.TypeString,
  Optional: true,
  Computed: true,
  Default: "AUTO_GENERATE_VIA_API",
},

You could then recognize the sentinel from your provider and properly omit the domain_name parameter from your upstream API call.

Of course this approach has limitations - it's another layer of semantics within the provider and it gets more difficult with non-string types (e.g. impossible with bools), but I believe it should suffice in most real-world cases.

...doesn't solve the larger problem of GetOk having an identity crisis: despite the Ok semantics, it doesn't actually behave the same way that, for instance, a Go map does, where the Ok parameter truly tells you if the value existed in the map. And I do think that there is value in having GetOk truly tell you whether a value exists in the configuration.

Perhaps that request should be moved to a separate issue, but I'd prefer that it not be dropped altogether.

Yep I think if you'd like to open up that discussion it would be cleaner to start it in a fresh issue.

[jefferai]

@phinze For the moment I'll leave it be. You're right about sentinel values (good call)...not ideal, but this is not likely something that is a widespread problem so I don't think it'd lead to too many checks in provider code. You also right that you can't do booleans, but I think this scenario with a boolean is even more unlikely. If this is something that does become a problem down the line, I'll open a new issue. Thanks!

[sparkprime]

You can either use a 'sentinel' value (actually this isn't a sentinel because it's not marking the end of a stream, but it is a value outside the regular range) or if you are already using all the values in the type, you can have an extra bool attribute to encode the extra state.

[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.