feat: Remove `NONE` validation method and set default to `null`

[magreenbaum]

Description

Closes: https://github.com/terraform-aws-modules/terraform-aws-acm/issues/133

Motivation and Context

https://github.com/hashicorp/terraform-provider-aws/pull/31455

Breaking Changes

Yes, validation_method default is now null since it's an optional argument and should be explicitly set to DNS or EMAIL if required.

How Has This Been Tested?

• [x] 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

[peritz]

Would it be better to create a non-breaking change which would just pass null to the underlying aws_acm_certificate resource if the argument passed was null? e.g.

validation_method = var.validation_method == "NONE" ? null : var.validation_method

[antonbabenko]

@magreenbaum The PR looks fine to me, but I think with the suggestion mentioned by @peritz above the PR will be even better.

What do you think about it?

[magreenbaum]

@peritz @antonbabenko I don't mind adding that but NONE is no longer supported by the underlying API. That would just add backward compatibility for those using NONE previously. It would still be a breaking change unless we want to keep DNS as the default validation_method and require null being explicitly set (this doesn't seem like standard practice for an optional parameter though).

I can update both if we want to make this a completely non-breaking change though.

[peritz]

You make a fair point @magreenbaum it's probably better to align with the underlying API, besides, since the underlying API changed the module has been broken trying to use "NONE" anyways, so I say go with your approach.

@antonbabenko what's your thoughts?

[antonbabenko]

This PR is included in version 4.4.0 :tada:

[cypher7682]

Chaps. This PR is backwards breaking on the interface. Why is this being released on a minor release? Do we even semver?

Almost everyone using DNS validation with this will have allowed the default to figure it out for them- when they apply, using BB safe versioning ~> 4.0, this will delete their validation records and allow certs to expire. This patch could have been made without needing to change the interface to the module.

Why would we not make this backwards compatible if we have the choice?

[cypher7682]

Just to be clear - you've released a minor version that will do the following when called with defaults in use:

module "cert" {
  source = "terraform-aws-modules/acm/aws"
  version = "~> 4.0"
  domain_name         = "xxx"
  zone_id             = data.aws_route53_zone.public-zone.zone_id
  wait_for_validation = true
}

Terraform will perform the following actions:

  # module.cert.aws_acm_certificate_validation.this[0] will be destroyed
  # (because index [0] is out of range for count)
  - resource "aws_acm_certificate_validation" "this" {
...
    }

  # module.cert.aws_route53_record.validation[0] will be destroyed
  # (because index [0] is out of range for count)
  - resource "aws_route53_record" "validation" {
...
    }

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

This could have been entirely avoided by releasing a major version, or actually just making the module perform as expected. null appears to perform the same job as NONE. Can we not just do this?

locals {
  validation = var.validation == NONE ? null : var.validation
}

This PR is entirely untested, but what would have been wrong with something like this? https://github.com/terraform-aws-modules/terraform-aws-acm/pull/139

[charlierm]

Totally agree with @cypher7682 on this, we're having to add validation or pin to the previous version. This contains breaking changes and should've been released as a major version or made backwards compatible.

[magreenbaum]

This PR is included in version 4.4.0 🎉

@antonbabenko can we change the release to 5.0.0 since it's a breaking change?

[cypher7682]

@antonbabenko can we change the release to 5.0.0 since it's a breaking change?

There's really no need to do that. This shouldn't have been a breaking change in the first place. I'm happy to run the tests on that example PR I linked and get it merged. But IMO the best route is to release a 4.4.1 which reverts the breaking change and squash 4.4.0 into it. The whole point of wrapping up the API in modules is that if the underlying API releases breaking changes, the module doesn't necessarily need to. There's no reason at all to "follow" someone else's conventions for the sake of it. Users don't give a hoot what the provider does, as long as the module 'just works :tm:'.

[cypher7682]

I've tested #139; the reinstatement of NONE, and reverting default back to DNS. It works fine. It's worth someone else giving it a once over, but AFAICS it should fix the interface on the module and the issue underlying provider/API. If this gets merged into 4.4.1 or something, it might be worth pulling 4.4.0 to make sure we don't start breaking people's validations if they accidentally fetch 4.4.0.

[github-actions[bot]]

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.