From ed3e6cb6c668f225b06450a4c59dee86f4250fe1 Mon Sep 17 00:00:00 2001 From: Jonathan Nagayoshi Date: Fri, 5 Mar 2021 15:17:44 -0300 Subject: [PATCH 1/2] #13605 added base64decode string before sending to aws-sdk-go, so certificate_wallet is not encoded twice --- .changelog/13605.txt | 3 +++ aws/resource_aws_dms_certificate.go | 7 ++++++- 2 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 .changelog/13605.txt diff --git a/.changelog/13605.txt b/.changelog/13605.txt new file mode 100644 index 00000000000..48056587b2b --- /dev/null +++ b/.changelog/13605.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_dms_certificate: Add base64 decode before sending to aws-sdk-go, so oracle-wallet is not encoded twice +``` diff --git a/aws/resource_aws_dms_certificate.go b/aws/resource_aws_dms_certificate.go index 1bae78a6b98..dca37ab8a41 100644 --- a/aws/resource_aws_dms_certificate.go +++ b/aws/resource_aws_dms_certificate.go @@ -1,6 +1,7 @@ package aws import ( + "encoding/base64" "fmt" "log" @@ -72,7 +73,11 @@ func resourceAwsDmsCertificateCreate(d *schema.ResourceData, meta interface{}) e request.CertificatePem = aws.String(pem.(string)) } if walletSet { - request.CertificateWallet = []byte(wallet.(string)) + byteArray, err := base64.StdEncoding.DecodeString(wallet.(string)) + if err != nil { + return fmt.Errorf("certificate_wallet is not a valid base64-encoded string") + } + request.CertificateWallet = byteArray } log.Println("[DEBUG] DMS import certificate:", request) From 21a251b1985df9b33d7fc0f04ee1887abe07bfbe Mon Sep 17 00:00:00 2001 From: Angie Pinilla Date: Tue, 16 Mar 2021 23:13:08 -0400 Subject: [PATCH 2/2] CR updates; base64encode in state; test coverage; linting --- .changelog/13605.txt | 3 - .changelog/17958.txt | 3 + aws/resource_aws_dms_certificate.go | 70 ++++++--- aws/resource_aws_dms_certificate_test.go | 142 +++++++++++++----- .../service/dms/oracle_wallet_certificate.pem | 10 ++ aws/validators.go | 19 --- aws/validators_test.go | 31 ---- website/docs/r/dms_certificate.html.markdown | 2 +- 8 files changed, 165 insertions(+), 115 deletions(-) delete mode 100644 .changelog/13605.txt create mode 100644 .changelog/17958.txt create mode 100644 aws/testdata/service/dms/oracle_wallet_certificate.pem diff --git a/.changelog/13605.txt b/.changelog/13605.txt deleted file mode 100644 index 48056587b2b..00000000000 --- a/.changelog/13605.txt +++ /dev/null @@ -1,3 +0,0 @@ -```release-note:bug -resource/aws_dms_certificate: Add base64 decode before sending to aws-sdk-go, so oracle-wallet is not encoded twice -``` diff --git a/.changelog/17958.txt b/.changelog/17958.txt new file mode 100644 index 00000000000..810bc032a80 --- /dev/null +++ b/.changelog/17958.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_dms_certificate: Correctly base64 decode `certificate_wallet` value +``` diff --git a/aws/resource_aws_dms_certificate.go b/aws/resource_aws_dms_certificate.go index dca37ab8a41..1acb17759f7 100644 --- a/aws/resource_aws_dms_certificate.go +++ b/aws/resource_aws_dms_certificate.go @@ -4,11 +4,13 @@ import ( "encoding/base64" "fmt" "log" + "regexp" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/awserr" dms "github.com/aws/aws-sdk-go/service/databasemigrationservice" + "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags" ) @@ -29,10 +31,15 @@ func resourceAwsDmsCertificate() *schema.Resource { Computed: true, }, "certificate_id": { - Type: schema.TypeString, - Required: true, - ForceNew: true, - ValidateFunc: validateDmsCertificateId, + Type: schema.TypeString, + Required: true, + ForceNew: true, + ValidateFunc: validation.All( + validation.StringLenBetween(1, 255), + validation.StringMatch(regexp.MustCompile("^[a-zA-Z][a-zA-Z0-9-]+$"), "must start with a letter, only contain alphanumeric characters and hyphens"), + validation.StringDoesNotMatch(regexp.MustCompile(`--`), "cannot contain two consecutive hyphens"), + validation.StringDoesNotMatch(regexp.MustCompile(`-$`), "cannot end in a hyphen"), + ), }, "certificate_pem": { Type: schema.TypeString, @@ -53,9 +60,10 @@ func resourceAwsDmsCertificate() *schema.Resource { func resourceAwsDmsCertificateCreate(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).dmsconn + certificateID := d.Get("certificate_id").(string) request := &dms.ImportCertificateInput{ - CertificateIdentifier: aws.String(d.Get("certificate_id").(string)), + CertificateIdentifier: aws.String(certificateID), Tags: keyvaluetags.New(d.Get("tags").(map[string]interface{})).IgnoreAws().DatabasemigrationserviceTags(), } @@ -63,31 +71,29 @@ func resourceAwsDmsCertificateCreate(d *schema.ResourceData, meta interface{}) e wallet, walletSet := d.GetOk("certificate_wallet") if !pemSet && !walletSet { - return fmt.Errorf("Must set either certificate_pem and certificate_wallet.") + return fmt.Errorf("Must set either certificate_pem or certificate_wallet for DMS Certificate (%s)", certificateID) } if pemSet && walletSet { - return fmt.Errorf("Cannot set both certificate_pem and certificate_wallet.") + return fmt.Errorf("Cannot set both certificate_pem and certificate_wallet for DMS Certificate (%s)", certificateID) } if pemSet { request.CertificatePem = aws.String(pem.(string)) } if walletSet { - byteArray, err := base64.StdEncoding.DecodeString(wallet.(string)) + certWallet, err := base64.StdEncoding.DecodeString(wallet.(string)) if err != nil { - return fmt.Errorf("certificate_wallet is not a valid base64-encoded string") + return fmt.Errorf("error Base64 decoding certificate_wallet for DMS Certificate (%s): %w", certificateID, err) } - request.CertificateWallet = byteArray + request.CertificateWallet = certWallet } - log.Println("[DEBUG] DMS import certificate:", request) - _, err := conn.ImportCertificate(request) if err != nil { - return err + return fmt.Errorf("error creating DMS certificate (%s): %w", certificateID, err) } - d.SetId(d.Get("certificate_id").(string)) + d.SetId(certificateID) return resourceAwsDmsCertificateRead(d, meta) } @@ -103,12 +109,24 @@ func resourceAwsDmsCertificateRead(d *schema.ResourceData, meta interface{}) err }, }, }) + + if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, dms.ErrCodeResourceNotFoundFault) { + log.Printf("[WARN] DMS Certificate (%s) not found, removing from state", d.Id()) + d.SetId("") + return nil + } + if err != nil { - if dmserr, ok := err.(awserr.Error); ok && dmserr.Code() == "ResourceNotFoundFault" { - d.SetId("") - return nil + return fmt.Errorf("error reading DMS Certificate (%s): %w", d.Id(), err) + } + + if response == nil || len(response.Certificates) == 0 || response.Certificates[0] == nil { + if d.IsNewResource() { + return fmt.Errorf("error reading DMS Certificate (%s): not found", d.Id()) } - return err + log.Printf("[WARN] DMS Certificate (%s) not found, removing from state", d.Id()) + d.SetId("") + return nil } err = resourceAwsDmsCertificateSetState(d, response.Certificates[0]) @@ -151,10 +169,16 @@ func resourceAwsDmsCertificateDelete(d *schema.ResourceData, meta interface{}) e CertificateArn: aws.String(d.Get("certificate_arn").(string)), } - log.Printf("[DEBUG] DMS delete certificate: %#v", request) - _, err := conn.DeleteCertificate(request) - return err + + if err != nil { + if tfawserr.ErrCodeEquals(err, dms.ErrCodeResourceNotFoundFault) { + return nil + } + return fmt.Errorf("error deleting DMS Certificate (%s): %w", d.Id(), err) + } + + return nil } func resourceAwsDmsCertificateSetState(d *schema.ResourceData, cert *dms.Certificate) error { @@ -167,7 +191,7 @@ func resourceAwsDmsCertificateSetState(d *schema.ResourceData, cert *dms.Certifi d.Set("certificate_pem", cert.CertificatePem) } if cert.CertificateWallet != nil && len(cert.CertificateWallet) != 0 { - d.Set("certificate_wallet", string(cert.CertificateWallet)) + d.Set("certificate_wallet", base64Encode(cert.CertificateWallet)) } return nil diff --git a/aws/resource_aws_dms_certificate_test.go b/aws/resource_aws_dms_certificate_test.go index 44a268d7eb5..cf902dbf89f 100644 --- a/aws/resource_aws_dms_certificate_test.go +++ b/aws/resource_aws_dms_certificate_test.go @@ -6,9 +6,9 @@ import ( "github.com/aws/aws-sdk-go/aws" dms "github.com/aws/aws-sdk-go/service/databasemigrationservice" + "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" ) @@ -19,12 +19,12 @@ func TestAccAWSDmsCertificate_basic(t *testing.T) { resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, Providers: testAccProviders, - CheckDestroy: dmsCertificateDestroy, + CheckDestroy: testAccCheckAWSDmsCertificateDestroy, Steps: []resource.TestStep{ { - Config: dmsCertificateConfig(randId), + Config: testAccAWSDmsCertificateConfig(randId), Check: resource.ComposeTestCheckFunc( - checkDmsCertificateExists(resourceName), + testAccAWSDmsCertificateExists(resourceName), resource.TestCheckResourceAttrSet(resourceName, "certificate_arn"), ), }, @@ -37,6 +37,52 @@ func TestAccAWSDmsCertificate_basic(t *testing.T) { }) } +func TestAccAWSDmsCertificate_disappears(t *testing.T) { + resourceName := "aws_dms_certificate.dms_certificate" + randId := acctest.RandString(8) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: nil, + Steps: []resource.TestStep{ + { + Config: testAccAWSDmsCertificateConfig(randId), + Check: resource.ComposeTestCheckFunc( + testAccAWSDmsCertificateExists(resourceName), + testAccCheckResourceDisappears(testAccProvider, resourceAwsDmsCertificate(), resourceName), + ), + ExpectNonEmptyPlan: true, + }, + }, + }) +} + +func TestAccAWSDmsCertificate_CertificateWallet(t *testing.T) { + resourceName := "aws_dms_certificate.dms_certificate" + rName := acctest.RandomWithPrefix("tf-acc-test") + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSDmsCertificateDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSDmsCertificateConfigCertificateWallet(rName), + Check: resource.ComposeTestCheckFunc( + testAccAWSDmsCertificateExists(resourceName), + resource.TestCheckResourceAttrSet(resourceName, "certificate_wallet"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + func TestAccAWSDmsCertificate_tags(t *testing.T) { resourceName := "aws_dms_certificate.dms_certificate" randId := acctest.RandString(8) @@ -44,12 +90,12 @@ func TestAccAWSDmsCertificate_tags(t *testing.T) { resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, Providers: testAccProviders, - CheckDestroy: dmsCertificateDestroy, + CheckDestroy: testAccCheckAWSDmsCertificateDestroy, Steps: []resource.TestStep{ { Config: testAccAWSDmsCertificateConfigTags1(randId, "key1", "value1"), Check: resource.ComposeAggregateTestCheckFunc( - checkDmsCertificateExists(resourceName), + testAccAWSDmsCertificateExists(resourceName), resource.TestCheckResourceAttr(resourceName, "tags.%", "1"), resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1"), ), @@ -62,7 +108,7 @@ func TestAccAWSDmsCertificate_tags(t *testing.T) { { Config: testAccAWSDmsCertificateConfigTags2(randId, "key1", "value1updated", "key2", "value2"), Check: resource.ComposeAggregateTestCheckFunc( - checkDmsCertificateExists(resourceName), + testAccAWSDmsCertificateExists(resourceName), resource.TestCheckResourceAttr(resourceName, "tags.%", "2"), resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1updated"), resource.TestCheckResourceAttr(resourceName, "tags.key2", "value2"), @@ -71,7 +117,7 @@ func TestAccAWSDmsCertificate_tags(t *testing.T) { { Config: testAccAWSDmsCertificateConfigTags1(randId, "key2", "value2"), Check: resource.ComposeAggregateTestCheckFunc( - checkDmsCertificateExists(resourceName), + testAccAWSDmsCertificateExists(resourceName), resource.TestCheckResourceAttr(resourceName, "tags.%", "1"), resource.TestCheckResourceAttr(resourceName, "tags.key2", "value2"), ), @@ -80,27 +126,40 @@ func TestAccAWSDmsCertificate_tags(t *testing.T) { }) } -func dmsCertificateDestroy(s *terraform.State) error { +func testAccCheckAWSDmsCertificateDestroy(s *terraform.State) error { for _, rs := range s.RootModule().Resources { if rs.Type != "aws_dms_certificate" { continue } - err := checkDmsCertificateExists(rs.Primary.ID) - if err == nil { - return fmt.Errorf("Found a certificate that was not destroyed: %s", rs.Primary.ID) + conn := testAccProvider.Meta().(*AWSClient).dmsconn + + output, err := conn.DescribeCertificates(&dms.DescribeCertificatesInput{ + Filters: []*dms.Filter{ + { + Name: aws.String("certificate-id"), + Values: []*string{aws.String(rs.Primary.ID)}, + }, + }, + }) + + if tfawserr.ErrCodeEquals(err, dms.ErrCodeResourceNotFoundFault) { + continue + } + + if err != nil { + return fmt.Errorf("error reading DMS Certificate (%s): %w", rs.Primary.ID, err) + } + + if output != nil && len(output.Certificates) != 0 { + return fmt.Errorf("DMS Certificate (%s) still exists", rs.Primary.ID) } } return nil } -func checkDmsCertificateExists(n string) resource.TestCheckFunc { - providers := []*schema.Provider{testAccProvider} - return checkDmsCertificateExistsWithProviders(n, &providers) -} - -func checkDmsCertificateExistsWithProviders(n string, providers *[]*schema.Provider) resource.TestCheckFunc { +func testAccAWSDmsCertificateExists(n string) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[n] if !ok { @@ -110,33 +169,31 @@ func checkDmsCertificateExistsWithProviders(n string, providers *[]*schema.Provi if rs.Primary.ID == "" { return fmt.Errorf("No ID is set") } - for _, provider := range *providers { - // Ignore if Meta is empty, this can happen for validation providers - if provider.Meta() == nil { - continue - } - - conn := provider.Meta().(*AWSClient).dmsconn - _, err := conn.DescribeCertificates(&dms.DescribeCertificatesInput{ - Filters: []*dms.Filter{ - { - Name: aws.String("certificate-id"), - Values: []*string{aws.String(rs.Primary.ID)}, - }, + + conn := testAccProvider.Meta().(*AWSClient).dmsconn + + output, err := conn.DescribeCertificates(&dms.DescribeCertificatesInput{ + Filters: []*dms.Filter{ + { + Name: aws.String("certificate-id"), + Values: []*string{aws.String(rs.Primary.ID)}, }, - }) + }, + }) - if err != nil { - return fmt.Errorf("DMS certificate error: %v", err) - } - return nil + if err != nil { + return fmt.Errorf("error reading DMS Certificate (%s): %w", rs.Primary.ID, err) } - return fmt.Errorf("DMS certificate not found") + if output == nil || len(output.Certificates) == 0 { + return fmt.Errorf("DMS Certificate (%s) not found", rs.Primary.ID) + } + + return nil } } -func dmsCertificateConfig(randId string) string { +func testAccAWSDmsCertificateConfig(randId string) string { return fmt.Sprintf(` resource "aws_dms_certificate" "dms_certificate" { certificate_id = "tf-test-dms-certificate-%[1]s" @@ -145,6 +202,15 @@ resource "aws_dms_certificate" "dms_certificate" { `, randId) } +func testAccAWSDmsCertificateConfigCertificateWallet(rName string) string { + return fmt.Sprintf(` +resource "aws_dms_certificate" "dms_certificate" { + certificate_id = %q + certificate_wallet = filebase64("testdata/service/dms/oracle_wallet_certificate.pem") +} +`, rName) +} + func testAccAWSDmsCertificateConfigTags1(randId, tagKey1, tagValue1 string) string { return fmt.Sprintf(` resource "aws_dms_certificate" "dms_certificate" { diff --git a/aws/testdata/service/dms/oracle_wallet_certificate.pem b/aws/testdata/service/dms/oracle_wallet_certificate.pem new file mode 100644 index 00000000000..0e20b50a826 --- /dev/null +++ b/aws/testdata/service/dms/oracle_wallet_certificate.pem @@ -0,0 +1,10 @@ +-----BEGIN CERTIFICATE----- +MIIBsTCCARoCAQAwDQYJKoZIhvcNAQEEBQAwITEfMB0GA1UEAxMWZ2JyMzAxMzkudWsub3JhY2xl +LmNvbTAeFw0xNjA1MTExMTQzMzFaFw0yNjA1MDkxMTQzMzFaMCExHzAdBgNVBAMTFmdicjMwMTM5 +LnVrLm9yYWNsZS5jb20wgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAKH8G8sFS6l0llu+RMfl +7Yt+Ppw8J0PfDEDbTGP5wtsrs/22dUCipU9l+vif1VgSPLE2UPJbGM8tQzTC6UYbBtWHe4CshmvD +EVlcIMsEFvD7a5Q+P45jqNSEtV9VdbGyxaD6i5Y/Smd+B87FcQQCX54LaI9BJ8SZwmPXgDweADLf +AgMBAAEwDQYJKoZIhvcNAQEEBQADgYEAai742jfNYYTKMq2xxRygGJGn1LhpFenHvuHLBvnTup1N +nZOBwBi4VxW3CImvwONYcCEFp3E1SRswS5evlfIfruCZ1xQBoUNei3EJ6O3OdKeRRp2E+muXEtfe +U+jwUE+SzpnzfpI23Okl2vo8Q7VHrSalxE2KEhAzC1UYX7ZYp1U= +-----END CERTIFICATE----- \ No newline at end of file diff --git a/aws/validators.go b/aws/validators.go index d5c20782d5d..4b7cd06f940 100644 --- a/aws/validators.go +++ b/aws/validators.go @@ -1211,25 +1211,6 @@ func validateSfnStateMachineName(v interface{}, k string) (ws []string, errors [ return } -func validateDmsCertificateId(v interface{}, k string) (ws []string, es []error) { - val := v.(string) - - if len(val) > 255 { - es = append(es, fmt.Errorf("%q must not be longer than 255 characters", k)) - } - if !regexp.MustCompile("^[a-zA-Z][a-zA-Z0-9-]+$").MatchString(val) { - es = append(es, fmt.Errorf("%q must start with a letter, only contain alphanumeric characters and hyphens", k)) - } - if strings.Contains(val, "--") { - es = append(es, fmt.Errorf("%q must not contain consecutive hyphens", k)) - } - if strings.HasSuffix(val, "-") { - es = append(es, fmt.Errorf("%q must not end in a hyphen", k)) - } - - return -} - func validateDmsEndpointId(v interface{}, k string) (ws []string, es []error) { val := v.(string) diff --git a/aws/validators_test.go b/aws/validators_test.go index e9941893c64..084c7f7d519 100644 --- a/aws/validators_test.go +++ b/aws/validators_test.go @@ -1308,37 +1308,6 @@ func TestValidateDmsEndpointId(t *testing.T) { } } -func TestValidateDmsCertificateId(t *testing.T) { - validIds := []string{ - "tf-test-certificate-1", - "tfTestEndpoint", - } - - for _, s := range validIds { - _, errors := validateDmsCertificateId(s, "certificate_id") - if len(errors) > 0 { - t.Fatalf("%q should be a valid certificate id: %v", s, errors) - } - } - - invalidIds := []string{ - "tf_test_certificate_1", - "tf.test.certificate.1", - "tf test certificate 1", - "tf-test-certificate-1!", - "tf-test-certificate-1-", - "tf-test-certificate--1", - "tf-test-certificate-1tf-test-certificate-1tf-test-certificate-1tf-test-certificate-1tf-test-certificate-1tf-test-certificate-1tf-test-certificate-1tf-test-certificate-1tf-test-certificate-1tf-test-certificate-1tf-test-certificate-1tf-test-certificate-1tf-test-certificate-1tf-test-certificate-1tf-test-certificate-1", - } - - for _, s := range invalidIds { - _, errors := validateDmsEndpointId(s, "certificate_id") - if len(errors) == 0 { - t.Fatalf("%q should not be a valid certificate id: %v", s, errors) - } - } -} - func TestValidateDmsReplicationInstanceId(t *testing.T) { validIds := []string{ "tf-test-replication-instance-1", diff --git a/website/docs/r/dms_certificate.html.markdown b/website/docs/r/dms_certificate.html.markdown index 8ab27b88edd..03eeab39011 100644 --- a/website/docs/r/dms_certificate.html.markdown +++ b/website/docs/r/dms_certificate.html.markdown @@ -37,7 +37,7 @@ The following arguments are supported: - Must contain from 1 to 255 alphanumeric characters and hyphens. * `certificate_pem` - (Optional) The contents of the .pem X.509 certificate file for the certificate. Either `certificate_pem` or `certificate_wallet` must be set. -* `certificate_wallet` - (Optional) The contents of the Oracle Wallet certificate for use with SSL. Either `certificate_pem` or `certificate_wallet` must be set. +* `certificate_wallet` - (Optional) The contents of the Oracle Wallet certificate for use with SSL, provided as a base64-encoded String. Either `certificate_pem` or `certificate_wallet` must be set. * `tags` - (Optional) A map of tags to assign to the resource. ## Attributes Reference