terraform-aws-modules / terraform-aws-acm

Terraform module to create AWS ACM resources πŸ‡ΊπŸ‡¦
https://registry.terraform.io/modules/terraform-aws-modules/acm/aws
Apache License 2.0
182 stars 229 forks source link

validation method is broken when validation is not required #103

Closed dudicoco closed 2 years ago

dudicoco commented 2 years ago

Description

https://github.com/terraform-aws-modules/terraform-aws-acm/commit/a9a3c2394df74de5235f6bbba260f186194c27f8 made validation_method a mandatory variable via variable validation: https://github.com/terraform-aws-modules/terraform-aws-acm/blob/a9a3c2394df74de5235f6bbba260f186194c27f8/variables.tf#L48-L51

We have certificates which were imported into Terraform therfor we didn't have validation_method defined until now. With the new changes, when setting validation_method = "NONE" the following plan is generated:

Terraform will perform the following actions:

  # module.acm.aws_acm_certificate.this[0] must be replaced
+/- resource "aws_acm_certificate" "this" {
      ~ arn                       = "arn:aws:acm:eu-central-1:xxxx:certificate/xxxx" -> (known after apply)
      ~ domain_validation_options = [] -> (known after apply)
      ~ id                        = "arn:aws:acm:eu-central-1:xxxx:certificate/xxxx" -> (known after apply)
      ~ status                    = "ISSUED" -> (known after apply)
      ~ subject_alternative_names = [] -> (known after apply)
      ~ validation_emails         = [
          - "xxxx@example.com",
        ] -> (known after apply)
      ~ validation_method         = "EMAIL" -> "NONE" # forces replacement
        # (2 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

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

Versions

Reproduction Code [Required]

provider "aws" {
  region = var.region
}

module "acm" {
  source  = "terraform-aws-modules/acm/aws"
  version = "3.4.0"

  domain_name               = var.domain_name
  subject_alternative_names = var.subject_alternative_names
  zone_id                   = var.route53_zone_id

  validation_method                  = var.validate_certificate ? "DNS" : "NONE"
  wait_for_validation                = false
  validation_allow_overwrite_records = false

  tags = {
    Name        = var.name
    Provisioner = "terraform"
  }
}
antonbabenko commented 2 years ago

Thanks for reporting this issue!

Email validation is usually happening outside of Terraform and thus it was not implemented by this module. I have updated the module now and verified that it works by running the code in examples/complete-email-validation and by importing created certificate.

validation_method should be set to "EMAIL" to match the imported value.

Fixed in #104

antonbabenko commented 2 years ago

This issue has been resolved in version 3.4.1 :tada:

dudicoco commented 2 years ago

Hi @antonbabenko,

I think there was a misunderstanding of the issue, I do not want to manage the validation method via terraform, I didn't import it as well. So my exception when setting validation_method = "NONE" is for no diff in the plan.

As I've mentioned before, we didn't have validation_method defined at all until now because we don't want to manage the validation via terraform. Using the new release still shows the same diff when setting validation_method = "NONE".

antonbabenko commented 2 years ago

The ACM certificate you have was validated using email. In the plan there is: ~ validation_method = "EMAIL" -> "NONE" # forces replacement

You need to set validation_method to "EMAIL" to prevent the plan from diffing.

dudicoco commented 2 years ago

@antonbabenko some of our certificates have DNS validation and the validation was done outside of terraform, in that case the plan will show a diff:

Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # module.acm.aws_route53_record.validation[0] will be created
  + resource "aws_route53_record" "validation" {
      + allow_overwrite = false
      + fqdn            = (known after apply)
      + id              = (known after apply)
      + name            = "xxxx"
      + records         = [
          + "xxxx",
        ]
      + ttl             = 60
      + type            = "CNAME"
      + zone_id         = "xxxx"
    }

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

In addition, even if there wouldn't be a diff for the DNS validation, it would still require us to go over all of our certificates and add validation_method with the correct value (DNS/EMAIL).

I think a more proper solution would be to revert to the previous behavior which allows not to specify validation_method, either by removing the variable validation for validation_method or by somehow allowing it to be null: https://github.com/terraform-aws-modules/terraform-aws-acm/blob/a9a3c2394df74de5235f6bbba260f186194c27f8/variables.tf#L48-L51

github-actions[bot] commented 1 year ago

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.