Implement aws_acm_certificate flag to query for most recent certificate #1837

dagoonline · 2017-10-08T19:51:18Z

Fixes: #546

When the flag is set to true, it returns only one certificate when several are found, the most recent based on NotBefore field in the certificate.

make testacc TEST=./aws TESTARGS='-run=TestAccAwsAcmCertificateDataSource_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAcmCertificateDataSource_ -timeout 120m
=== RUN   TestAccAwsAcmCertificateDataSource_noMatchReturnsError
--- PASS: TestAccAwsAcmCertificateDataSource_noMatchReturnsError (19.56s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	19.597s

Updated: fix documentation after review by @fertapric (thanks!). Also include a more efficient way to avoid unneeded api calls.

dagoonline · 2017-10-25T16:15:42Z

@radeksimko any feedback on this one?

hroussez · 2017-11-01T21:09:58Z

This PR would play nice with our code, when a certificate must be re-issued or updated (add/remove domains). Any update on this?

dagoonline · 2017-11-14T17:29:17Z

Any news regarding this PR?

cmacrae · 2017-11-23T11:23:37Z

Great work @dagoonline 👏

Would love to see this in an upcoming release some time soon! Sucks having to use static ARNs at the moment.

loivis · 2018-01-19T20:03:08Z

any attention on this? fix conflicts and merge?

…re field

…erts

dagoonline · 2018-01-20T21:10:34Z

Master rebased and conflicts fixed. Please @radeksimko let me know if I can help making the process of merging this PR easier somehow.

bflad · 2018-01-25T04:48:26Z

I'm in the process of reviewing this PR and will pick it back up tomorrow

bflad

Hi @dagoonline! Awesome work here and we are excited to get this functionality into the AWS provider. 😄 If you do not have time to finalize this PR, please let us know and we will finish it up with your commits.

I left some comments below which will help the codebase here. I'm also going to leave you the below code snippet as something to consider to simplify the logic here for filtering (please note: not fully tested).

Please reach out with any questions and let us know when it is good to review again.

// after listing the certificates
	if len(arns) == 0 {
		return fmt.Errorf("No certificate for domain %q found in this region", target)
	}

	filterMostRecent := d.Get("most_recent").(bool)
	filterTypes, filterTypesOk := d.GetOk("types")

	if !filterMostRecent && !filterTypesOk && len(arns) > 1 {
		return fmt.Errorf("Multiple certificates for domain %q found in this region", target)
	}

	var matchedCertificate *acm.CertificateDetail
	typesStrings := expandStringList(filterTypes.([]interface{}))

	for _, arn := range arns {
		input := &acm.DescribeCertificateInput{
			CertificateArn: aws.String(arn),
		}
		log.Printf("[DEBUG] Describing ACM Certificate: %s", input)
		output, err := conn.DescribeCertificate(input)
		if err != nil {
			return fmt.Errorf("Error describing ACM certificate: %s", err)
		}
		certificate := output.Certificate
		if filterTypesOk {
			for _, certType := range typesStrings {
				if *certificate.Type == *certType {
					// We do not have a candidate certificate
					if matchedCertificate == nil {
						matchedCertificate = certificate
						break
					}
					// At this point, we already have a candidate certificate
					// Check if we are filtering by most recent and update if necessary
					if filterMostRecent && (*certificate.NotBefore).After(*matchedCertificate.NotBefore) {
						matchedCertificate = certificate
						break
					}
					// Now we have multiple candidate certificates and we only allow one certificate
					return fmt.Errorf("Multiple certificates for domain %q found in this region", target)
				}
			}
			continue
		}
		// We do not have a candidate certificate
		if matchedCertificate == nil {
			matchedCertificate = certificate
			continue
		}
		// At this point, we already have a candidate certificate
		// Check if we are filtering by most recent and update if necessary
		if filterMostRecent && (*certificate.NotBefore).After(*matchedCertificate.NotBefore) {
			matchedCertificate = certificate
			continue
		}
		// Now we have multiple candidate certificates and we only allow one certificate
		return fmt.Errorf("Multiple certificates for domain %q found in this region", target)
	}

	if matchedCertificate == nil {
		return fmt.Errorf("No certificate for domain %q found in this region", target)
	}

	d.SetId(time.Now().UTC().String())
	d.Set("arn", matchedCertificate.CertificateArn)

bflad · 2018-01-25T03:07:41Z

aws/data_source_aws_acm_certificate.go

+				Type:     schema.TypeBool,
+				Optional: true,
+				Default:  false,
+				Elem:     &schema.Schema{Type: schema.TypeString},


Extraneous Elem here

bflad · 2018-01-25T03:09:25Z

aws/data_source_aws_acm_certificate.go

 		},
 	}
 }

+type arnData struct {


Can we use the SDK provided *acm.CertificateDetail instead of our own custom type? https://docs.aws.amazon.com/sdk-for-go/api/service/acm/#CertificateDetail

bflad · 2018-01-25T03:16:36Z

aws/data_source_aws_acm_certificate.go

+	notBefore *time.Time
+}
+
+func describeCertificate(arn *arnData, conn *acm.ACM) (*acm.DescribeCertificateOutput, error) {


Can you update this function to have a signature and functionality like the following?
describeAcmCertificate(arn *string, conn *acm.ACM) (*acm.CertificateDetail, error)

Although if we squash the logic in the most_recent portion of the code to only ever have one place to call this function, its logic can be moved down into where the code is being used.

I've tried to squash the logic in only one place but unfortunately there are two places when this code comes in handy. I've kept the function, with the signature you've proposed.

bflad · 2018-01-25T03:22:07Z

aws/data_source_aws_acm_certificate.go

+			if mr.notBefore == nil {
+				description, err := describeCertificate(mr, conn)
+				if err != nil {
+					return errwrap.Wrapf("Error describing certificates: {{err}}", err)


Can you switch these to fmt.Errorf() please? We do not need to use errwrap.Wrapf() for its extra context in these cases. (Sorry I know there's some other code here which is already using errwrap 🙁 .)

dagoonline · 2018-01-28T20:44:47Z

Hi @bflad. Thanks for the review! I've applied the feedback and reviewed the proposed code in your comment. Mostly applied as it is cleaner. Many thanks for the suggestions!

This needs testing, I've checked base cases and also tested it with makeacc but would be great to test all cases. Unfortunately I don't have currently the setup to pass through all scenarios. I will do if not done already when I have some spare time.

bflad · 2018-01-29T08:17:42Z

I can check this against my personal account tomorrow. 🕐 (ACM sets NotBefore to the current day's midnight when a certificate is approved, so need to approve certificates over two days.)

bflad · 2018-01-29T08:27:43Z

Actually that brings up an interesting point. NotBefore only applies to ISSUED certificates. Do we need to check against CreatedAt instead for other statuses?

for CERTIFICATE_ARN in $(aws acm list-certificates | jq -r '.CertificateSummaryList[].CertificateArn'); do aws acm describe-certificate --certificate-arn $CERTIFICATE_ARN | jq '.Certificate | {Status: .Status, CreatedAt: .CreatedAt, NotBefore: .NotBefore}'; done
{
  "Status": "ISSUED",
  "CreatedAt": 1517208091,
  "NotBefore": 1517184000
}
{
  "Status": "ISSUED",
  "CreatedAt": 1517208234,
  "NotBefore": 1517184000
}
{
  "Status": "ISSUED",
  "CreatedAt": 1517208323,
  "NotBefore": 1517184000
}
{
  "Status": "PENDING_VALIDATION",
  "CreatedAt": 1517208375,
  "NotBefore": null
}

dagoonline · 2018-01-29T12:09:25Z

Interesting indeed. From AWS certificate detail documentation seems that CreatedAt field exists only when the certificate has been issued by Amazon, setting the field Type to AMAZON_ISSUED, so CreatedAt field won't exist in IMPORTED certificates.

I think this will cover both, ignoring the other cases. Maybe arising an error there would fit better?:

if filterMostRecent {
	if (*certificate.Status == "ISSUED" && (*certificate.NotBefore).After(*matchedCertificate.NotBefore)) ||
		(*certificate.Status != "ISSUED" && *certificate.Type == "AMAZON_ISSUED" && (*certificate.CreatedAt).After(*matchedCertificate.CreatedAt)) {
		matchedCertificate = certificate
		break
	} else {
		//Ignore other cases and keep trying.
		continue
	}
}

bflad · 2018-01-30T20:34:01Z

It might be easiest to split it into its own method to contain all the necessary logic in this case, something like:

func mostRecentAcmCertificate(i, j *acm.CertificateDetail) *acm.CertificateDetail {
	if (*mostRecentAcmCertificateTime(i)).After(*mostRecentAcmCertificateTime(j)) {
		return i
	}
	return j
}

func mostRecentAcmCertificateTime(certificate *acm.CertificateDetail) *time.Time {
	if certificate.NotBefore != nil {
		return certificate.NotBefore
	}
	if certificate.CreatedAt != nil {
		return certificate.CreatedAt
	}
	return certificate.ImportedAt
}

Then we can just use:

matchedCertificate = mostRecentAcmCertificate(certificate, matchedCertificate)

Curious about your thoughts! Unfortunately something like this needs to be implemented here otherwise it'll simply crash on nil deferences. I think something like the function above would catch all the various imported/pending/created/issued states.

bflad · 2018-02-01T13:26:03Z

@dagoonline also at any point let me know if you do not have time to finish this up - I know the back and forth can be difficult. You have done an awesome job and we are pretty close. 👍

dagoonline · 2018-02-01T14:12:45Z

@bflad I won't have time until weekend so go ahead 👍 Many thanks!

About the method splitting, It won't fit for me because I think it can end up comparing CreatedAt and NotBefore fields, messing things up.

The proposed code in my last comment should never end up in nil deferences, since all the cases all covered: when the certificate has status ISSUED will always have the NotBefore field, as a valid certificate. Otherwise, if the state has status different than ISSUED but was created in ACM (therefore set AMAZON_ISSUED as Type), it will always have the CreatedAt field. Otherwise we can ignore or fail the check since the certificate we're comparing has no way to be compared.

Also I would like to give a thought on trying not have the code to check most recent certificate twice, once when the flag for types filtering is set and once when not. Anyway this is just a matter of cleanliness so no big concerns here.

bflad · 2018-02-01T14:55:31Z

Ah, you are right! 😄 Even IMPORTED certificates will have NotBefore.

I will try to finish this up today as well as the acceptance testing usable with an environment variable instead of just expecting errors in all cases.

Thanks again for all your work here. We (the community and maintainers) really appreciate it!

…ng non-ISSUED certificates and add initial acceptance testing

bflad

Adjusted with the CreatedAt handling and tested it manually against some ACM certificates in my personal account. This is ready to ship! 🚢 ⛵️ 🛥

make testacc TEST=./aws TESTARGS='-run=TestAccAwsAcmCertificateDataSource'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAcmCertificateDataSource -timeout 120m
=== RUN   TestAccAwsAcmCertificateDataSource_singleIssued
--- PASS: TestAccAwsAcmCertificateDataSource_singleIssued (22.33s)
=== RUN   TestAccAwsAcmCertificateDataSource_multipleIssued
--- PASS: TestAccAwsAcmCertificateDataSource_multipleIssued (15.51s)
=== RUN   TestAccAwsAcmCertificateDataSource_noMatchReturnsError
--- PASS: TestAccAwsAcmCertificateDataSource_noMatchReturnsError (3.88s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	41.776s

bflad · 2018-02-09T22:33:32Z

This has been released in terraform-provider-aws version 1.9.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-08T17:19:36Z

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!

radeksimko added the enhancement Requests to existing resources that expand the functionality or scope. label Oct 9, 2017

dagoonline force-pushed the dagoonline/546_query-most-recent-certificate branch from dd3b7a0 to f9b1f64 Compare October 10, 2017 13:01

radeksimko added the size/L Managed by automation to categorize the size of a PR. label Nov 15, 2017

David González added 7 commits January 20, 2018 22:03

Update documentation with missing implemented field

8e6df0b

Documentation for 'most_recent' flag

18f17cc

Test for coverage based on guidelines

69645a6

Implement most_recent flag to filter matching certificates by NotBefo…

607f620

…re field

Fill base case fields when nil.

cbb12a8

Fix documentation and include more examples

ab4396c

Improve efficiency by only using most_recent when there are several c…

ed070f2

…erts

dagoonline force-pushed the dagoonline/546_query-most-recent-certificate branch from f9b1f64 to ed070f2 Compare January 20, 2018 21:08

bflad added the service/acm Issues and PRs that pertain to the acm service. label Jan 25, 2018

bflad self-assigned this Jan 25, 2018

bflad reviewed Jan 25, 2018

View reviewed changes

Apply feedback

a75e3b4

ghost added the size/L Managed by automation to categorize the size of a PR. label Jan 28, 2018

bflad mentioned this pull request Jan 31, 2018

New Resources: aws_acm_certificate and aws_acm_certificate_validation #2813

Merged

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 1, 2018

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Feb 1, 2018

bflad added this to the v1.9.0 milestone Feb 1, 2018

data-source/aws_acm_certificate: Prevent crash on most_recent filteri…

961c68d

…ng non-ISSUED certificates and add initial acceptance testing

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 1, 2018

bflad approved these changes Feb 1, 2018

View reviewed changes

bflad merged commit 59dda9d into hashicorp:master Feb 1, 2018

bflad added a commit that referenced this pull request Feb 1, 2018

Update CHANGELOG for #1837

d9492e9

ghost locked and limited conversation to collaborators Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement aws_acm_certificate flag to query for most recent certificate #1837

Implement aws_acm_certificate flag to query for most recent certificate #1837

dagoonline commented Oct 8, 2017 •

edited

Loading

dagoonline commented Oct 25, 2017

hroussez commented Nov 1, 2017

dagoonline commented Nov 14, 2017

cmacrae commented Nov 23, 2017

loivis commented Jan 19, 2018

dagoonline commented Jan 20, 2018 •

edited

Loading

bflad commented Jan 25, 2018

bflad left a comment

bflad Jan 25, 2018

bflad Jan 25, 2018

bflad Jan 25, 2018

dagoonline Jan 28, 2018

bflad Jan 25, 2018

dagoonline commented Jan 28, 2018

bflad commented Jan 29, 2018

bflad commented Jan 29, 2018

dagoonline commented Jan 29, 2018 •

edited

Loading

bflad commented Jan 30, 2018

bflad commented Feb 1, 2018

dagoonline commented Feb 1, 2018 •

edited

Loading

bflad commented Feb 1, 2018

bflad left a comment

bflad commented Feb 9, 2018

ghost commented Apr 8, 2020

Implement aws_acm_certificate flag to query for most recent certificate #1837

Implement aws_acm_certificate flag to query for most recent certificate #1837

Conversation

dagoonline commented Oct 8, 2017 • edited Loading

dagoonline commented Oct 25, 2017

hroussez commented Nov 1, 2017

dagoonline commented Nov 14, 2017

cmacrae commented Nov 23, 2017

loivis commented Jan 19, 2018

dagoonline commented Jan 20, 2018 • edited Loading

bflad commented Jan 25, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad Jan 25, 2018

Choose a reason for hiding this comment

bflad Jan 25, 2018

Choose a reason for hiding this comment

bflad Jan 25, 2018

Choose a reason for hiding this comment

dagoonline Jan 28, 2018

Choose a reason for hiding this comment

bflad Jan 25, 2018

Choose a reason for hiding this comment

dagoonline commented Jan 28, 2018

bflad commented Jan 29, 2018

bflad commented Jan 29, 2018

dagoonline commented Jan 29, 2018 • edited Loading

bflad commented Jan 30, 2018

bflad commented Feb 1, 2018

dagoonline commented Feb 1, 2018 • edited Loading

bflad commented Feb 1, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented Feb 9, 2018

ghost commented Apr 8, 2020

dagoonline commented Oct 8, 2017 •

edited

Loading

dagoonline commented Jan 20, 2018 •

edited

Loading

dagoonline commented Jan 29, 2018 •

edited

Loading

dagoonline commented Feb 1, 2018 •

edited

Loading