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

[WIP] New Resource: aws_acm_certificate (+ changes to data source to wait for certificate issuing) #2801

Closed

Conversation

flosell
Copy link
Contributor

@flosell flosell commented Dec 29, 2017

This is a first draft trying to implement automated ACM certificate issuing based on my suggestions in #2418.

Here is what it does:

  • Add a aws_acm_certificate resource that requests a new certificate and returns once validationOptions (the information what needs to be done to validate ownership of the domain) are available (for some reason, those aren't immediately available). Only supports DNS validation at this point
  • Add an option to aws_acm_certificate data source to wait until a matching certificate can be found
  • Add an option to aws_acm_certificate to search for arn instead of domain so we match exactly the certificate we requested from acm
  • Add an acceptance test that runs the whole certificate issuing flow (using route53 for DNS validation)

Here is what's still to do:

  • Add support for tagging
  • Implement and test imports
  • Update Documentation
  • Support E-Mail validation

Here are a few things I'm unsure about:

  • Should wait_until_present and wait_until_present_timeout be two fields or one field where 0 means "don't wait"?
  • The acceptance test needs a valid, publicly available domain to validate certificate requests. Is this a problem for CI? Should this be configurable and disabled by default so contributors aren't annoyed when working in unrelated parts?
  • Is it a good idea to model waiting for certificate issuing in a data source? If one is not careful (e.g. forgets the dependency on the route53 record), the data source starts polling in terraform plan without any chance of ever succeeding. If you have the dependency, it looks like terraform will always produce a non-empty plan for it (depends_on always triggers data source read terraform#11806).
    As an alternative, waiting for certificate issuing could also be implemented as a separate resource:
    resource "aws_acm_certificate_validation" "cert" {
      certificate_arn = "${aws_acm_certificate.cert.certificate_arn}"
      validation_record = "${aws_route53_record.cert_validation.fqdn}" # This wouldn't strictly be necessary but it can enforce a dependency
    }
    This would also allow us to do additional checks (e.g. validating that the DNS record is correctly set) and would probably resolve the issues with the data source. It also introduces new ones though: It adds clutter to the resource-list and lead to confusion since it doesn't conform to any resource in AWS. E.g. which resource destroy will trigger certificate deletion: acm_certificate_validation or acm_certificate?
  • AWS does not expect subject_alternative_names to contain values when requesting a certificate but DescribeCertificate always includes the domain_name in subject_alternative_names. I'm currently filtering this so terraform doesn't detect this as a change. Is there a better way to do this?

I'm not a golang or terraform provider expert so any feedback is welcome!

@jen20 jen20 added enhancement Requests to existing resources that expand the functionality or scope. question A question about existing functionality; most questions are re-routed to discuss.hashicorp.com. labels Dec 29, 2017
@flosell
Copy link
Contributor Author

flosell commented Dec 31, 2017

Update: I implemented a variation of this that uses a resource instead of a data source to implement waiting for the issued certificate: #2813

To me, it feels cleaner but I'll wait for feedback before continuing and then close one of the PRs

@flosell
Copy link
Contributor Author

flosell commented Jan 14, 2018

Haven't received much feedback yet but the resource-based approach still seems more promising. Closing this PR in favour of #2813 and continue work there until we decide otherwise.

@flosell flosell closed this Jan 14, 2018
@cemo
Copy link

cemo commented Jan 14, 2018

Gentle ping @apparentlymart.

@ghost
Copy link

ghost commented Apr 8, 2020

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. question A question about existing functionality; most questions are re-routed to discuss.hashicorp.com.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants