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.

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

ignore_changes assumes that the setting was originally created and set from within Terraform #26401

Closed
nexxai opened this issue Sep 28, 2020 · 11 comments
Labels
bug explained a Terraform Core team member has described the root cause of this issue in code working as designed confirmed as reported and closed because the behavior is intended

Comments

@nexxai
Copy link
Contributor

nexxai commented Sep 28, 2020

Terraform Version

Terraform v0.13.3

Terraform Configuration Files

lifecycle {
  ignore_changes = [
    app_settings["EXAMPLE_SETTING"]
  ]
}

Debug Output

n/a

Crash Output

n/a

Expected Behavior

  1. Assume "EXAMPLE_SETTING" is a setting that is created and set by a tool outside of Terraform
  2. Setting "ignore_changes" on that specific setting should mean that Terraform should not attempt to change it

Actual Behavior

Because Terraform did not create the setting, it is not being managed and so it cannot be ignored. As described here, it appears to be an issue with this line of code assuming that the setting was already being managed, when one of the main use cases for ignoring changes is to deal with configuration changes from systems outside of Terraform.

A suggestion here is to create and use placeholder values within the Terraform script, except that if these placeholders are used in modules where external systems may not touch all instances of that module, those placeholder values stick around which can cause their own problems, making this not a feasible solution.

Instead of assuming the change is "Managed", when detailing a specific change to ignore, Terraform should simply compare the list of changes to ignore (regardless of original creation source) and if it exists within that list, ignore it. There shouldn't be an assumption that a setting is being managed before subsequently ignoring it. Explaining that in English would basically be saying: "I need you to know about this thing so that you can not know about this thing." It's a contradiction.

Steps to Reproduce

terraform plan or terraform apply

Additional Context

n/a

References

hashicorp/terraform-provider-azurerm#4321

@nexxai nexxai added bug new new issue not yet triaged labels Sep 28, 2020
@jbardin jbardin added explained a Terraform Core team member has described the root cause of this issue in code working as designed confirmed as reported and closed because the behavior is intended and removed new new issue not yet triaged labels Sep 29, 2020
@jbardin
Copy link
Member

jbardin commented Sep 29, 2020

Hi @nexxai,

In the next version of Terraform, setting ignore_changes on a field that is not in the configuration is likely to result in an error, preventing users from setting it when there is nothing it could do. You can see the full set of changes in #26387, which should not alter the behavior of the provider in this case, it only solidifies in the internal handling of ignore_changes.

The ignore_changes lifecycle argument is mean to apply to configuration only. The only values that should arrive in state which are not in the configuration are Computed, and in that case there would be no planned change since it is a value expected to be set by the provider.

Looking at that resource's source in the provider, it appears app_settings is a simple map(string), which is Optional in the schema. A provider is not allowed to alter the this field in the state because it is not computed, and a check of the logs when using this resource would likely find warnings similar to [WARN] Provider "registry.terraform.io/hashicorp/azurerm" produced an unexpected new value .... For backwards compatibility reasons we cannot enforce these errors in core universally, though they will be hard errors when providers move to a new SDK.

@jbardin jbardin closed this as completed Sep 29, 2020
@nexxai
Copy link
Contributor Author

nexxai commented Sep 29, 2020

@jbardin Maybe I did a bad job explaining what I'm looking for.

In the example, app_settings would exist in the Terraform script as well as specific setting app_settings["TERRAFORM"], but a specific setting (e.g. app_settings["EXTERNAL"]) has been created and set outside of Terraform. All I want to do is specify that Terraform should ignore any changes to app_settings["EXTERNAL"] but still keep watching over app_settings["TERRAFORM"], like I would expect the ignore_changes setting to do. I just want Terraform to pretend it doesn't exist and go on its merry way. Is that not possible?

@jbardin
Copy link
Member

jbardin commented Sep 29, 2020

Hi @nexxai,

Thanks, I think see now what you're asking for, but unfortunately that's not something that Terraform is designed to handle. The root cause of the problem here is that the provider is modifying app_settings, which it should not be doing at all. While the ignore_changes feature does allow users to hide some "drift" that a misbehaving provider may cause, it's not the intended use of that feature.

@nexxai
Copy link
Contributor Author

nexxai commented Sep 29, 2020

@jbardin And I get that what we're asking for wasn't the original intent, but what I don't get is why something like this couldn't be added? It may not have been designed or intended for that use originally, but as you can see in the comments of the multiple issues I referenced, it's not an unheard of request, and it doesn't seem like it should be that difficult to implement.

If I was better at Go, I'd submit a PR myself, but the ask is simply an extra step before it diffs the currently known/managed state with the requested/declared state in the .tf file to say "if you see these entries, just knock them out of the array that you're about to compare". I realize PHP is mostly a joke to any "real" developer, but even in PHP, we would just unset() or array_splice() to take that item out of the array (which would effectively "ignore" it), prior to comparing it to the managed state, basically like giving Terraform a defined, intentional blind spot. And it's not like someone could accidentally trip and fall into enabling this setting, so the risk is extremely low; they would need to specifically define what they want to ignore, just like they do now.

The point of ignoring these changes is so that we can let Terraform continue on its way doing its Terraform thing without getting unnecessarily confused at things that it doesn't need to worry about. I am a huge Terraform evangelist and speak its praises to literally anyone who will listen, but there are some things that in certain cases it either can't handle (like a new feature that a particular provider doesn't yet support) or another business unit not wanting Terraform to touch a piece of their process for whatever reason. All we're looking for is a way to place nice with those BUs while we poke and prod at them to believe in the ways of Terraform and ditch their external tools and/or buy time until the relevant providers are updated to support Feature X.

Is there any chance you'd reconsider? I know there are a lot of people (myself very much included) who would be very appreciative.

@jbardin
Copy link
Member

jbardin commented Sep 29, 2020

@nexxai, I understand the frustration here, but the change really needs to happen on the provider side. There's shouldn't be a need for this type of behavior in the current ignore_changes implementation, because this situation should not happen with a correctly behaving provider.

I think there's a subtle difference in what you you're asking for and what is actually required here. If I understand correctly being able to ignore an individual map index which is not listed in the config should suffice to work around this problem. That specific change is likely to land in 0.14 in a follow up to the linked PR.

This does not mean you will be able to ignore computed attributes (which should be the only way a provider can set state values), nor will it prevent this type of provider behavior from becoming an error in the future.

@nexxai
Copy link
Contributor Author

nexxai commented Sep 29, 2020

@jbardin

So I've re-read the code in that PR a few times (as I said, I suck at Go) and I think it's going to be the nail in the coffin for what we're looking for, because at least in this specific case (azurerm_app_service -> app_settings), it's a computed property:

"app_settings": {
	Type:     schema.TypeMap,
	Optional: true,
	Computed: true,
	Elem: &schema.Schema{
		Type: schema.TypeString,
	},
},

which if I understand your PR correctly, means that not only will it silently ignore the entry (as it would right now) but it will actually throw an error if we have:

app_settings = {
  this_setting_managed_by_terraform = true
}

lifecycle {
  ignore_changes = [
    app_settings["this_setting_managed_externally"]
  ]
}

right? I really hope I'm reading that wrong, but I don't think I am.

@jbardin
Copy link
Member

jbardin commented Sep 29, 2020

Having the attribute also be Optional makes it valid to set in the configuration, so it should not produce an error. (I'm going to rework that validation check to be even more specific, or remove if that doesn't work before merging).

The azurerm_app_service however is slightly different from from the issue you've linked, since that was referring to azurerm_function_app where app_settings cannot be computed. The current SDK that the azurerm provider uses does not have the ability to discern whether a computed+optional value can be modified or not, so they may often overwrite values incorrectly. This PR should not effect the situation one way or the other, but I have a feeling an upcoming PR to ignore unset map keys will allow you to work around the provider's behavior.

@nexxai
Copy link
Contributor Author

nexxai commented Sep 29, 2020

@jbardin The stupid thing is that they not only operate the same way, behind the scenes, they're literally the same thing. I am going to submit a PR for that issue to make azurerm_function_app -> app_settings a computed property because both App Services and Function Apps should be functionally identical.

WRT your comment about the "feeling about an upcoming PR": is that hinting that it's soon? I'm not trying to rush things, I just need to know what I should be telling my boss; like whether I tell him "it's coming, maybe in the next month?" or "it's coming, maybe in the next 6 months" so he can make a determination on how we move forward.

@jbardin
Copy link
Member

jbardin commented Sep 29, 2020

@nexxai

The PR I was thinking of is now up at #26421. This is something that would be merged for 0.14.

@nexxai
Copy link
Contributor Author

nexxai commented Sep 29, 2020

Oh man, that's perfect and exactly what we're looking for. I know on some of the providers' repos, they have general target dates for version releases; does this main repo have one somewhere that I can check out?

katbyte pushed a commit to hashicorp/terraform-provider-azurerm that referenced this issue Oct 4, 2020
Function Apps should have computed App Settings just like App Services do, since they are effectively two different offshoots of the same resource type behind the scenes.

Also required as discussed in [this thread](hashicorp/terraform#26401 (comment))
@ghost
Copy link

ghost commented Oct 29, 2020

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.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug explained a Terraform Core team member has described the root cause of this issue in code working as designed confirmed as reported and closed because the behavior is intended
Projects
None yet
Development

No branches or pull requests

2 participants