-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
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
dd3b7a0
to
f9b1f64
Compare
@radeksimko any feedback on this one? |
This PR would play nice with our code, when a certificate must be re-issued or updated (add/remove domains). Any update on this? |
Any news regarding this PR? |
Great work @dagoonline 👏 Would love to see this in an upcoming release some time soon! Sucks having to use static ARNs at the moment. |
any attention on this? fix conflicts and merge? |
f9b1f64
to
ed070f2
Compare
Master rebased and conflicts fixed. Please @radeksimko let me know if I can help making the process of merging this PR easier somehow. |
I'm in the process of reviewing this PR and will pick it back up tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Type: schema.TypeBool, | ||
Optional: true, | ||
Default: false, | ||
Elem: &schema.Schema{Type: schema.TypeString}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extraneous Elem
here
}, | ||
} | ||
} | ||
|
||
type arnData struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
notBefore *time.Time | ||
} | ||
|
||
func describeCertificate(arn *arnData, conn *acm.ACM) (*acm.DescribeCertificateOutput, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if mr.notBefore == nil { | ||
description, err := describeCertificate(mr, conn) | ||
if err != nil { | ||
return errwrap.Wrapf("Error describing certificates: {{err}}", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙁 .)
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 |
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.) |
Actually that brings up an interesting point.
|
Interesting indeed. From AWS certificate detail documentation seems that I think this will cover both, ignoring the other cases. Maybe arising an error there would fit better?:
|
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 |
@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. 👍 |
@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 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 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. |
Ah, you are right! 😄 Even 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
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! |
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.
Updated: fix documentation after review by @fertapric (thanks!). Also include a more efficient way to avoid unneeded api calls.