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

fix: Marked AMI id as nonsensitive #321

Conversation

kevingunn-wk
Copy link
Contributor

@kevingunn-wk kevingunn-wk commented Mar 14, 2023

Description

When running terraform plan - the ami is hidden and marked as sensitive. This happens whether or not an AMI Id is provided or ami_ssm_parameter is used. This is because aws_ssm_parametervalue is by default a sensitive value.

Example:

 # module.ec2.aws_instance.this[0] will be created
  + resource "aws_instance" "this" {
      + ami                                  = (sensitive value)
      + arn                                  = (known after apply)
      + associate_public_ip_address          = true
      + availability_zone                    = (known after apply)
      + cpu_core_count                       = (known after apply)
      + cpu_threads_per_core                 = (known after apply)
      + disable_api_stop                     = (known after apply)
      + disable_api_termination              = (known after apply)
      + ebs_optimized                        = (known after apply)
      + get_password_data                    = false
      + host_id                              = (known after apply)
      + host_resource_group_arn              = (known after apply)
      + iam_instance_profile                 = (known after apply)

Motivation and Context

When running terraform plan, it would be nice to know the AMI ID, especially when a replace is triggered. If I am replacing the instance because of an AMI change, I want to make sure the correct AMI ID is being used. This is especially helpful when using ami_ssm_parameter, to make sure the correct paramter value is used.

I also think it might be useful to include the AMI as the output. When using ami_ssm_parameter, the actual AMI Id is not obvious, so including it in the output makes it easy to identify which AMI was used from the parameter store.

Breaking Changes

N/A

How Has This Been Tested?

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

Ran terraform plan after making the change

 # module.ec2_t3_unlimited.aws_instance.this[0] will be created
  + resource "aws_instance" "this" {
      + ami                                  = "ami-05247819264504af0"
      + arn                                  = (known after apply)
      + associate_public_ip_address          = true
      + availability_zone                    = (known after apply)
      + cpu_core_count                       = (known after apply)
      + cpu_threads_per_core                 = (known after apply)
      + disable_api_stop                     = (known after apply)
      + disable_api_termination              = (known after apply)
      + ebs_optimized                        = (known after apply)
      + get_password_data                    = false
      + host_id                              = (known after apply)
      + host_resource_group_arn              = (known after apply)
      + iam_instance_profile                 = (known after apply)

@kevingunn-wk kevingunn-wk changed the title AMI Id does not need to be sensitive Feature: AMI Id does not need to be sensitive Mar 14, 2023
@kevingunn-wk kevingunn-wk changed the title Feature: AMI Id does not need to be sensitive feat: AMI Id does not need to be sensitive Mar 14, 2023
@github-actions
Copy link

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Apr 21, 2023
@kevingunn-wk
Copy link
Contributor Author

Not stale - would still be nice to see this merged so that AMI Ids are not flagged as sensitive.

@antonbabenko antonbabenko changed the title feat: AMI Id does not need to be sensitive fix: Marked AMI id as nonsensitive Apr 21, 2023
@antonbabenko antonbabenko merged commit 1ae1d5c into terraform-aws-modules:master Apr 21, 2023
1 check passed
antonbabenko pushed a commit that referenced this pull request Apr 21, 2023
### [4.3.1](v4.3.0...v4.3.1) (2023-04-21)

### Bug Fixes

* Marked AMI id as nonsensitive ([#321](#321)) ([1ae1d5c](1ae1d5c))
@antonbabenko
Copy link
Member

This PR is included in version 4.3.1 🎉

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants