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

Sensitive variable with null default causes unnecessary plan to update in-place #32864

Closed
tatnat opened this issue Mar 16, 2023 · 5 comments · Fixed by #32892
Closed

Sensitive variable with null default causes unnecessary plan to update in-place #32864

tatnat opened this issue Mar 16, 2023 · 5 comments · Fixed by #32892
Assignees
Labels
bug core explained a Terraform Core team member has described the root cause of this issue in code

Comments

@tatnat
Copy link

tatnat commented Mar 16, 2023

Terraform Version

❱❱❱ terraform version
Terraform v1.4.0
on darwin_arm64
+ provider registry.terraform.io/hashicorp/aws v4.58.0
+ provider registry.terraform.io/hashicorp/local v2.4.0

Terraform Configuration Files

variable "test_env" {
  type      = string
  default   = null
  sensitive = true
}

terraform {
  required_version = ">= 0.13"

  required_providers {
    aws = {
      source  = "hashicorp/aws"
      version = "~> 4.57"
    }
  }
}

# Configure the AWS Provider
provider "aws" {
  region = "us-west-1"
}

resource "aws_lambda_function" "this" {
  function_name = "test-function"
  role          = "************"
  image_uri     = "************"
  package_type  = "Image"

  environment {
    variables = {
      TEST_ENV = var.test_env
      # TEST_ENV = var.test_env == null ? null : var.test_env # this actually works
      FOO = "bar"
    }
  }
}

Debug Output

https://gist.github.com/tatnat/60f079e4868b08beb61824afc7575ede

Expected Behavior

terraform plan or terraform apply should not detect any changes.

Actual Behavior

terraform plan or terraform apply detects changes with the variable that has default = null as well as sensitive = true when the default is used. When sensitive = false then this behavior does not occur.

Example:

❱❱❱ terraform plan
aws_lambda_function.this: Refreshing state... [id=test-function]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # aws_lambda_function.this will be updated in-place
  ~ resource "aws_lambda_function" "this" {
        id                             = "test-function"
        tags                           = {}
        # (20 unchanged attributes hidden)

        # (3 unchanged blocks hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

Note: You didn't use the -out option to save this plan, so Terraform can't guarantee to take exactly these actions if you run "terraform apply" now.

Steps to Reproduce

  1. terraform init
  2. terraform plan
  3. terraform plan

Additional Context

Under the environment and variables section, if conditional logic is used to set the variable such as:

TEST_ENV = var.test_env == null ? null : var.test.env

Then the expected behavior is actually produced (no changes detected).

Example:

❱❱❱ terraform plan
aws_lambda_function.this: Refreshing state... [id=test-function]

No changes. Your infrastructure matches the configuration.

Terraform has compared your real infrastructure against your configuration and found no differences, so no changes are needed.

References

No response

@tatnat tatnat added bug new new issue not yet triaged labels Mar 16, 2023
@kmoe
Copy link
Member

kmoe commented Mar 16, 2023

Thanks for this - we will look into it. The weird plan output is a case of #31887, but it looks like there might be something happening here with the sensitive value - perhaps a "sensitive null" vs a "non-sensitive null".

@kmoe kmoe removed the new new issue not yet triaged label Mar 16, 2023
@apparentlymart
Copy link
Member

I'm not 100% sure what's going on here either but I think it's worth noting that because this seems to be a map from the provider's perspective this is not a situation where setting it to null would be the same as omitting it entirely, because the provider will be able to observe the presence of the element in the map even though its value is a null.

Therefore I would expect the UI to describe the map value like this when the value is null:

  variables = {
    FOO      = "bar"
    TEST_ENV = null
  }

(I've illustrated what it would look like when creating the object for the first time above; of course if the whole value hasn't changed for an in-place update or a replace then as usual Terraform should hide the unchanged elements behind an "unchanged elements hidden" comment.)

I don't think this answer can fully explain what happened here but I wonder if the diff renderer is incorrectly using the normal rules for object types like the resource config as a whole -- where an attribute being set to null is how we represent "unset" -- also for map values, where that interpretation would be misleading due to the keys of a map being a dynamic part of its value rather than a part of its static type.

There seems to also be a quirk in the logic which decides whether the planned state differs from the prior state, making this appear as an "update in-place" even though nothing seems to be changing. That decision gets made in Terraform Core by comparing the prior state with the planned state, and my top-of-head theory (without looking at the code) is that something is incorrectly using cty.RawEquals to compare instead of cty.Equals, and is therefore inadvertently treating a sensitive value as not equal to a nonsensitive value that is otherwise equivalent. (Similar cause as #32859, but probably in a different part of plans/objchange than caused that one.)

@jbardin jbardin self-assigned this Mar 16, 2023
@jbardin
Copy link
Member

jbardin commented Mar 16, 2023

Yes, the difference in the config is is that sensitive(null) is different from just the null which could be assigned via the conditional expression.

If there is a RawEquals internally it was probably intentional however, because we need to record changes in sensitivity as part of the value when creating a change, even if the underlying value is not changing.

@jbardin
Copy link
Member

jbardin commented Mar 16, 2023

Ok, I have all the behavior traced out here.

As I mentioned in the previous comment, the value returned by var.test_env is different from the value returned by var.test_env == null ? null : var.test.env, in that the former is sensitive, while the latter is not. If the provider were behaving correctly, this would always present a change in the UI regardless of the way the value were assigned.

The reason there is no change shown with a literal null, is that the provider using the legacy SDK filters out null values from the map (it cannot differentiate between unset and null). This means that when you add the null map value the provider ignores it and returns a value which matches the prior state.

When you add null value marked as sensitive by inserting the variable directly, the provider creates the same plan, but because core recorded the configuration as having a change in sensitivity, it must still be marked as a change in order to notify the user (the provider has no say in config sensitivity). The additional field however is now missing because the provider dropped it, so the plan ends displaying no changes being made.

In this particular case the provider's behavior is technically allowed, as the prior state can be returned in lieu of the proposed state when there are no functional changes. The provider does not handle this correctly in any other case however, and will return the wrong value even when changes will be recorded, but Terraform will only warn about this in the logs due to the compatibility guarantees for legacy providers.

I think we can actually catch this by filtering out the recorded marks for paths not present in the planned value, because there is a valid case where a provider is allowed to return a plan which does not exactly match the config.

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug core explained a Terraform Core team member has described the root cause of this issue in code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants