Add a no_store meta-parameter

[sarneaud]

This partially addresses #516

no_store is like ignore_changes but stronger. A no_store attribute
isn't stored in the state file at all.

Existing end users and providers shouldn't be relying on ignore_changes
attributes being stored in the state file, so in principle this could be
implemented without creating a new parameter. However, that wouldn't
leave any migration path for anything affected by a significant change
in behaviour. The new parameter has a name that better reflects its
more general usage.

This PR deprecates ignore_changes. no_store has been implemented with
the assumption that the ignore_changes code will eventually be deleted.

Because "not storing in state" is a functional requirement and not just
an implementation detail, a check for this was added to the end-to-end
test.

[apparentlymart]

Hi @sarneaud! Thanks for working on this.

It sounds like the use-case you have in mind here is to tell Terraform that although a resource exports a particular attribute, you have no use for it and would thus prefer that Terraform did not retain the record of it.

For something this deep in core we generally prefer to have a design discussion before jumping to implementation, since experience tells us that changes to big assumptions like this often have knock-on effects on other parts of Terraform.

So with that said, I have some design questions I'd like to dig into before we dive into discussing the code itself:

1. It's not clear to me that this feature should replace ignore_changes. You asserted in your writeup here that users should not be interpolating things that are marked as ignore_changes, but I don't believe that this is true... it's still valid to refer to something that Terraform ignores for diffing purposes, since you can use the value that was true at the creation of the instance, or that has been subsequently updated by a refresh or other side-effect. Do you see something I'm missing here that led you to this conclusion?

2. Any value that isn't stored in the state must not be permitted in interpolation, since although interpolation doesn't always consult state, it can, and it's better for interpolation to always fail than to only fail in some cases. I would suggest that we detect any references to unstored attributes in interpolation and make it an immediate error, to be caught during validation; as you said, there's no good reason to use unstored attributes in interpolation anyway. For completeness, we should probably omit such values entirely from the interpolation scope, even if they'd otherwise be available from the diff. These together are likely to help mitigate unintended side-effects.

3. You touched on the interaction with terraform import in your docs here, and that raised a more general question for me: if a resource is created without this lifecycle attribute set and then it's later added, should any existing value in the state be removed? I lean towards yes here, but we need to be sure to do that only during apply since other actions should not apply changes made in configuration. For import in particular, since we now require a stub configuration to be present before a resource is imported we could consider honoring this lifecycle attribute, if present, during the import step too.

4. Sometimes providers depend on the state-stored values of particular attributes in their Refresh, Update and Delete operations. This is the trickiest problem, I think, since such dependencies are generally not documented because today they are implementation details. If a user were to mark one of these important attributes as non-stored, the provider code may act in an unpredictable way. I think before we could merge this we'd need to do a review of the main providers to look for such situations and consider the effect that ignoring their critical attributes would have, so that at minimum we can document these rough edges, and possibly consider making adjustments to smooth them out.

5. This is of course a minor, subjective one but since the other attributes in lifecycle have been verbs, it'd be nice to find a name for this that fits in well with the others. My first thought here was ignore_values, with the thinking that this is a stronger version of ignore_changes that discards the values themselves, as well as ignoring any changes to them for diffing purposes. What do you think?

That's all the thoughts I have to start. Thanks again for working on this, and sorry for the barrage of questions; I just want to make sure that the interactions with existing subsystems are explored and that we can minimize weird edge-cases that may cause problems for users of this feature.

[sarneaud]

Don't worry. I'm okay with reworking the PR, but I really wanted to push towards some kind of solution. (I originally raised this in a comment in #516.)

1. Well, I didn't assert that ignore_changes shouldn't be used with interpolations. I did say that users should be careful of leaking secret values with interpolations, if they're using no_store for security reasons, so maybe that's what you're thinking of. But you mention a case where interpolations are based on some initial value that's marked ignore_changes. Could that be reimplemented by putting the initial value into a variable or template that gets interpolated wherever it's needed? (Do you have any use cases in mind?)

2. There are some good reasons to put no_store values into interpolations (e.g., generating a secret in some resource and storing it in a Vault generic_secret), so this is definitely worth considering. Based on the "least surprise" principle, maybe we should require any attribute that depends on a no_store attribute to itself be marked no_store. I'll take a look at what can be implemented.

3. That sounds sensible. I suppose we should make sure it creates a diff. As for import, I wasn't aware that configs are now required, so I can revisit that.

4. Yeah, this is the main reason I created a new no_store instead of changing the behaviour of ignore_changes. There are so many combinations of possible uses, both from providers and end users. By creating a new parameter, users can opt in to no_store, and can revert to ignore_changes and file bug reports if they have problems. Perhaps we can change the wording of the docs to present it as experimental.

5. I don't have strong opinions on this. Just confirm the name and I'll update the PR.

Here are some use cases for no_store.

I'm currently working at a site that's generating AWS RDS instances with Terraform. The DB password is required for creation, but we don't want the password to be in the state file. As a kludge, the password is set based on uuid() and then automation outside Terraform resets the password using the AWS CLI. This wouldn't be necessary if the password could be excluded from the state file.

The Hashicorp Vault integration is really handy, except that any secrets managed with Terraform end up copied to the state file. This is a similar scenario to the RDS password problem.

[sidewinder12s]

I'd argue without this kind of parameter the Vault providers data source for generic secrets almost feels pointless. We are already limiting what we manage in Vault with Terraform because I almost don't see the point in using it if we have another system that just leaks everything out somewhere else. If we have some way of telling Terraform to not store values it retrieves, we at least can still use Vault as a datasource for secrets to pull at Terraform run and just place them in Vault out of band.

[rabbitfang]

One option for dealing with issues of provider support is for providers to specify which attributes can be no_store'd, i.e. providers have to opt-in in order for no_store to be used (either on a per-attribute basis, per-resource, or for the whole provider).

[mildwonkey]

Hi folks! I am working on addressing some older PRs - sorry for the long silence here!

Based on Martin's request for a larger design conversation, and the fact that this PR touches a lot of code that's changed greatly since terraform 0.12 has released, I am going to close this PR.

I encourage anyone interested in this feature to add a 👍 to #516 (or continue the conversation if you have new ideas to share). Thanks!

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