fix: Mark the output of the AMI ID as nonsensitive

dhoppe commented 1 year ago

Description

Using the latest version of this Terraform module and the AWS provider causes the following error message.

dhoppe at mac-mini in eu-west-1/tfs4jira/ec2 on  main
❯ terragrunt plan
data.aws_partition.current: Reading...
data.aws_ssm_parameter.this[0]: Reading...
data.aws_partition.current: Read complete after 0s [id=aws]
data.aws_iam_policy_document.assume_role_policy[0]: Reading...
data.aws_iam_policy_document.assume_role_policy[0]: Read complete after 0s [id=1256122602]
aws_iam_role.this[0]: Refreshing state... [id=tfs4jira-ec2-dev]
data.aws_ssm_parameter.this[0]: Read complete after 0s [id=/aws/service/ami-amazon-linux-latest/amzn2-ami-hvm-x86_64-gp2]
aws_iam_role_policy_attachment.this["CloudWatchAgentServerPolicy"]: Refreshing state... [id=tfs4jira-ec2-dev-20230321143309684800000004]
aws_iam_role_policy_attachment.this["AmazonSSMDirectoryServiceAccess"]: Refreshing state... [id=tfs4jira-ec2-dev-20230321143309486800000002]
aws_iam_role_policy_attachment.this["TFS4JiraKMSDecrypt"]: Refreshing state... [id=tfs4jira-ec2-dev-20230321165022847500000001]
aws_iam_role_policy_attachment.this["AmazonSSMManagedInstanceCore"]: Refreshing state... [id=tfs4jira-ec2-dev-20230321143309636100000003]
aws_iam_role_policy_attachment.this["AmazonEC2ReadOnlyAccess"]: Refreshing state... [id=tfs4jira-ec2-dev-20230321143309341500000001]
aws_iam_instance_profile.this[0]: Refreshing state... [id=tfs4jira-ec2-dev]
aws_instance.ignore_ami[0]: Refreshing state... [id=i-03962882275b387bf]

Planning failed. Terraform encountered an error while generating this plan.

╷
│ Error: Output refers to sensitive values
│
│   on outputs.tf line 146:
│  146: output "ami" {
│
│ To reduce the risk of accidentally exporting sensitive data that was
│ intended to be only internal, Terraform requires that any root module
│ output containing sensitive data be explicitly marked as sensitive, to
│ confirm your intent.
│
│ If you do intend to export this data, annotate the output value as
│ sensitive by adding the following argument:
│     sensitive = true
╵
ERRO[0010] Terraform invocation failed in /Users/dhoppe/Documents/customers/REDACTED/terragrunt-aws-atlassian/stacks/dev/eu-west-1/tfs4jira/ec2/.terragrunt-cache/IjGgvqSySUKtu-5Pyf61D24eh6U/pfgqyj3TsBEWff7a1El6tYu6LEE  prefix=[/Users/dhoppe/Documents/customers/REDACTED/terragrunt-aws-atlassian/stacks/dev/eu-west-1/tfs4jira/ec2]
ERRO[0010] 1 error occurred:
    * [/Users/dhoppe/Documents/customers/REDACTED/terragrunt-aws-atlassian/stacks/dev/eu-west-1/tfs4jira/ec2/.terragrunt-cache/IjGgvqSySUKtu-5Pyf61D24eh6U/pfgqyj3TsBEWff7a1El6tYu6LEE] exit status 1

Motivation and Context

I would like to be able to use this Terraform module again without causing any error messages. Since the AMI ID is not sensitive anyway, this change should not be a problem.

Breaking Changes

This change does not break backwards compatibility with the current major version.

How Has This Been Tested?

[ ] I have updated at least one of the examples/* to demonstrate and validate my change(s)
[x] I have tested and validated these changes using one or more of the provided examples/* projects
[x] I have executed pre-commit run -a on my pull request

dhoppe commented 1 year ago

@antonbabenko To be honest, I am pretty confused too. The problem only occurred with one of two modules.

Possibly the problem is caused by Terragrunt, although both modules get the AMI ID via a data source.

In any case, this was the only way to get rid of the error message. It feels wrong, but it does not hurt either. ;)

antonbabenko commented 1 year ago

The problem with using nonsensitive() in places where it should not be used can lead to errors like "Invalid value for "value" parameter: the given value is not sensitive, so this call is redundant."

More info - https://github.com/hashicorp/terraform/issues/31646

I'd rather find a better solution to the problem than wrap it like you do in this PR.

dhoppe commented 1 year ago

@antonbabenko I just passed the input values to a .auto.tfvars and used terraform plan to make sure that this issue is not caused by Terragrunt, but I get the same error message.

dhoppe at macbook-pro in terraform-aws-ec2-instance on  master [?] via 💠 default took 8s
❯ terraform plan
...
Plan: 0 to add, 3 to change, 0 to destroy.

Changes to Outputs:
  ~ instance_state                     = "running" -> "stopped"
  ~ tags_all                           = {
      - Contact            = "Dennis Hoppe"
      - Environment        = "dev"
      - Owner              = "SICO"
      - Project            = "Atlassian"
        # (13 unchanged attributes hidden)
    }
╷
│ Error: Output refers to sensitive values
│
│   on outputs.tf line 146:
│  146: output "ami" {
│
│ To reduce the risk of accidentally exporting sensitive data that was intended to be only internal, Terraform
│ requires that any root module output containing sensitive data be explicitly marked as sensitive, to confirm your
│ intent.
│
│ If you do intend to export this data, annotate the output value as sensitive by adding the following argument:
│     sensitive = true
╵
Releasing state lock. This may take a few moments...

The changes can be ignored because the EC2 instance was stopped by a lambda function.

I also can confirm that the input value of the AMI is not sensitive.

dhoppe at macbook-pro in dev/eu-west-1/aws-data on  main [?] took 4s
❯ terragrunt output -json | jq .aws_ami_tfs4jira_dev_image_id
{
  "sensitive": false,
  "type": "string",
  "value": "ami-010c18a20eec1a70a"
}

By the way, this error only occurs if I set ignore_ami_changes to true as I already mentioned at https://github.com/terraform-aws-modules/terraform-aws-ec2-instance/pull/331#issuecomment-1540301986.

antonbabenko commented 1 year ago

I've tried to reproduce the issue and made #340 but it just works as expected.

Could you please try to run the example and see if you can provide me with the failing example?

dhoppe commented 1 year ago

I use this Terraform module for two different use cases and in a total of three environments.

However, for some reason, only one of two use cases and two of three environments were affected.

So I did some investigation and there seems to be a correlation with the migration from resource "aws_instance" "this" to resource "aws_instance" "ignore_ami". Because after I deleted the resource from the statefile and imported it again, the problem disappeared.

@antonbabenko Sorry for wasting your time.

antonbabenko commented 1 year ago

Glad to hear that the problem is resolved! Also, now we have an example (aka test case) as code :)

github-actions[bot] commented 1 year ago

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

terraform-aws-modules / terraform-aws-ec2-instance