Allow AWS Provider ACM Certificate Data Source to Return TLS Certificate Material #24593

rizkybiz · 2022-05-05T16:13:45Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #0000

Output from acceptance testing:

$ make testacc TESTS=TestAccXXX PKG=ec2

...

…for use in other resources

github-actions

Welcome @rizkybiz 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTING guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

anGie44

Hi @rizkybiz, thanks again for this PR! It's off to a great start, just a couple notes to address in addition to a request to update the test file (internal/service/acm/certificate_data_source_test.go) with the following lines to check for the new expected values.

func TestAccACMCertificateDataSource_singleIssued(t *testing.T) {
	...

	resource.ParallelTest(t, resource.TestCase{
		PreCheck:   func() { acctest.PreCheck(t) },
		ErrorCheck: acctest.ErrorCheck(t, acm.EndpointsID),
		Providers:  acctest.Providers,
		Steps: []resource.TestStep{
			{
				Config: testAccCheckCertificateDataSourceConfig(domain),
				Check: resource.ComposeTestCheckFunc(
					//lintignore:AWSAT001
					resource.TestMatchResourceAttr(resourceName, "arn", arnRe),
					resource.TestCheckResourceAttr(resourceName, "status", acm.CertificateStatusIssued),
					resource.TestCheckResourceAttrSet(resourceName, "certificate"),
					resource.TestCheckResourceAttrSet(resourceName, "certificate_chain"),
				),
			},
			{
				Config: testAccCheckCertificateWithStatusDataSourceConfig(domain, acm.CertificateStatusIssued),
				Check: resource.ComposeTestCheckFunc(
					//lintignore:AWSAT001
					resource.TestMatchResourceAttr(resourceName, "arn", arnRe),
					resource.TestCheckResourceAttr(resourceName, "status", acm.CertificateStatusIssued),
					resource.TestCheckResourceAttrSet(resourceName, "certificate"),
					resource.TestCheckResourceAttrSet(resourceName, "certificate_chain"),
				),
			},
			{
				Config: testAccCheckCertificateWithTypesDataSourceConfig(domain, acm.CertificateTypeAmazonIssued),
				Check: resource.ComposeTestCheckFunc(
					//lintignore:AWSAT001
					resource.TestMatchResourceAttr(resourceName, "arn", arnRe),
					resource.TestCheckResourceAttrSet(resourceName, "certificate"),
					resource.TestCheckResourceAttrSet(resourceName, "certificate_chain"),
				),
			},
			{
				Config: testAccCheckCertificateWithMostRecentDataSourceConfig(domain, true),
				Check: resource.ComposeTestCheckFunc(
					//lintignore:AWSAT001
					resource.TestMatchResourceAttr(resourceName, "arn", arnRe),
					resource.TestCheckResourceAttrSet(resourceName, "certificate"),
					resource.TestCheckResourceAttrSet(resourceName, "certificate_chain"),
				),
			},
			{
				Config: testAccCheckCertificateWithMostRecentAndStatusDataSourceConfig(domain, acm.CertificateStatusIssued, true),
				Check: resource.ComposeTestCheckFunc(
					//lintignore:AWSAT001
					resource.TestMatchResourceAttr(resourceName, "arn", arnRe),
					resource.TestCheckResourceAttrSet(resourceName, "certificate"),
					resource.TestCheckResourceAttrSet(resourceName, "certificate_chain"),
				),
			},
			{
				Config: testAccCheckCertificateWithMostRecentAndTypesDataSourceConfig(domain, acm.CertificateTypeAmazonIssued, true),
				Check: resource.ComposeTestCheckFunc(
					//lintignore:AWSAT001
					resource.TestMatchResourceAttr(resourceName, "arn", arnRe),
					resource.TestCheckResourceAttrSet(resourceName, "certificate"),
					resource.TestCheckResourceAttrSet(resourceName, "certificate_chain"),
				),
			},
		},
	})
}

Reach out if you have any questions! I'm also happy to help contribute to the work here :)

anGie44 · 2022-05-11T17:37:40Z

internal/service/acm/certificate_data_source.go

+	}
+	output, err := conn.GetCertificate(&getCertInput)
+	if err != nil {
+		return err


similar to the error returned @ L196, let's format this error with some additional context to help operators debug potential errors e.g.

Suggested change

return err

return fmt.Errorf("error getting ACM Certificate (%s): %w, aws.StringValue(matchedCertificate.CertificateArn), err)

anGie44 · 2022-05-11T18:21:47Z

internal/service/acm/certificate_data_source.go

@@ -169,10 +177,19 @@ func dataSourceCertificateRead(d *schema.ResourceData, meta interface{}) error {
 		return fmt.Errorf("No certificate for domain %q found in this region", target)
 	}

+	// get the certificate data


on the off chance that a user tries to use this data source on a certificate that is not yet issued, say in a "PENDING_VALIDATION" state instead, the GetCertificate will error with a RequestInProgressException , so we can guard against that here by only making the API call iff the matchedCertificate.Status is ISSUED e.g.

var certOutput *acm.GetCertificateOutput if aws.StringValue(matchedCertificate.Status) == acm.CertificateStatusIssued { input := &acm.GetCertificateInput{ CertificateArn: matchedCertificate.CertificateArn, } certOutput, err = conn.GetCertificate(input) if err != nil { return fmt.Errorf("error getting ACM Certificate (%s): %w", aws.StringValue(matchedCertificate.CertificateArn), err) } } # ... if certOutput != nil { d.Set("certificate", certOutput.Certificate) d.Set("certificate_chain", certOutput.CertificateChain) } else { d.Set("certificate", nil) d.Set("certificate_chain", nil) }

anGie44 · 2022-05-11T18:25:02Z

internal/service/acm/certificate_data_source.go

@@ -25,6 +25,14 @@ func DataSourceCertificate() *schema.Resource {
 				Type:     schema.TypeString,
 				Computed: true,
 			},
+			"tls_certificate": {


nit: i think we can drop the tls_ prefix here as the API returns this value as Certificate

do you mind also adding the param to https://github.com/hashicorp/terraform-provider-aws/blob/main/website/docs/d/acm_certificate.html.markdown#attributes-reference ?

anGie44 · 2022-05-11T18:26:58Z

internal/service/acm/certificate_data_source.go

+				Type:     schema.TypeString,
+				Computed: true,
+			},
+			"tls_certificate_full_chain": {


similar comment: the API returns this value as CertificateChain so we can map that here to certificate_chain

do you mind also adding the param to https://github.com/hashicorp/terraform-provider-aws/blob/main/website/docs/d/acm_certificate.html.markdown#attributes-reference ?

… issued acm certificate

… attributes

…id a requestInProgressException for a non fully issued certificate

… the acm_certificate dat source docs.

rizkybiz · 2022-05-13T03:07:01Z

@anGie44 I believe I've taken care of all of your concerns. I appreciate the great suggestions, (basically fixed it all yourself) and let me know if there's anything else that needs to be done to get these changes merged.

anGie44

Changes looks great @rizkybiz ! Thanks so much 🚀

Output of subset of acceptance tests:

--- PASS: TestAccACMCertificateDataSource_singleIssued (49.25s)
--- PASS: TestAccACMCertificateDataSource_noMatchReturnsError (12.72s)

github-actions · 2022-05-13T17:25:27Z

This functionality has been released in v4.14.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

github-actions · 2022-06-13T02:37:26Z

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.

upd: allow aws provider ACM cert data source to return cert material …

ff2e4c2

…for use in other resources

github-actions bot added service/acm Issues and PRs that pertain to the acm service. needs-triage Waiting for first response or review from a maintainer. size/XS Managed by automation to categorize the size of a PR. labels May 5, 2022

github-actions bot reviewed May 5, 2022

View reviewed changes

justinretzolk added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels May 5, 2022

anGie44 self-assigned this May 10, 2022

anGie44 suggested changes May 11, 2022

View reviewed changes

rizkybiz added 4 commits May 12, 2022 22:34

upd: added checks for certificate and certificate_chain within single…

d5c7cf4

… issued acm certificate

upd: modified the names of the certificate and certificate full chain…

82cd96b

… attributes

upd: introduced a check for issued certificate status in order to avo…

b91b2be

…id a requestInProgressException for a non fully issued certificate

upd: added certificate and certificate_chain attribute definitions to…

7701f57

… the acm_certificate dat source docs.

rizkybiz changed the title ~~[WIP] Allow AWS Provider ACM Certificate Data Source to Return TLS Certificate Material~~ Allow AWS Provider ACM Certificate Data Source to Return TLS Certificate Material May 13, 2022

anGie44 approved these changes May 13, 2022

View reviewed changes

anGie44 added this to the v4.14.0 milestone May 13, 2022

Update CHANGELOG for hashicorp#24593

3be774c

anGie44 merged commit 6ce26f4 into hashicorp:main May 13, 2022

github-actions bot locked as resolved and limited conversation to collaborators Jun 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow AWS Provider ACM Certificate Data Source to Return TLS Certificate Material #24593

Allow AWS Provider ACM Certificate Data Source to Return TLS Certificate Material #24593

rizkybiz commented May 5, 2022

github-actions bot left a comment

anGie44 left a comment

anGie44 May 11, 2022

anGie44 May 11, 2022

anGie44 May 11, 2022

anGie44 May 11, 2022

anGie44 May 11, 2022

anGie44 May 11, 2022

rizkybiz commented May 13, 2022

anGie44 left a comment

github-actions bot commented May 13, 2022

github-actions bot commented Jun 13, 2022

	return err
	return fmt.Errorf("error getting ACM Certificate (%s): %w, aws.StringValue(matchedCertificate.CertificateArn), err)

Allow AWS Provider ACM Certificate Data Source to Return TLS Certificate Material #24593

Allow AWS Provider ACM Certificate Data Source to Return TLS Certificate Material #24593

Conversation

rizkybiz commented May 5, 2022

Community Note

github-actions bot left a comment

Choose a reason for hiding this comment

anGie44 left a comment

Choose a reason for hiding this comment

anGie44 May 11, 2022

Choose a reason for hiding this comment

anGie44 May 11, 2022

Choose a reason for hiding this comment

anGie44 May 11, 2022

Choose a reason for hiding this comment

anGie44 May 11, 2022

Choose a reason for hiding this comment

anGie44 May 11, 2022

Choose a reason for hiding this comment

anGie44 May 11, 2022

Choose a reason for hiding this comment

rizkybiz commented May 13, 2022

anGie44 left a comment

Choose a reason for hiding this comment

github-actions bot commented May 13, 2022

github-actions bot commented Jun 13, 2022