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 for ses domain identity Policy #5128

Merged
merged 2 commits into from
Jun 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions aws/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,7 @@ func Provider() terraform.ResourceProvider {
"aws_secretsmanager_secret_version": resourceAwsSecretsManagerSecretVersion(),
"aws_ses_active_receipt_rule_set": resourceAwsSesActiveReceiptRuleSet(),
"aws_ses_domain_identity": resourceAwsSesDomainIdentity(),
"aws_ses_domain_identity_policy": resourceAwsSesDomainIdentityPolicy(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two larger items:

  • Given that this resource can work with both SES Identity Policies for both domains and emails, we should consider naming it aws_ses_identity_policy as there would be no benefit to having separate resources between the two. The file, resource, and resource testing function names will need to be updated to reflect this change.
  • This new resource is missing resource documentation in a new website/docs/r/ses_identity_policy.html.markdown file and a website sidebar link in website/aws.erb

"aws_ses_domain_identity_verification": resourceAwsSesDomainIdentityVerification(),
"aws_ses_domain_dkim": resourceAwsSesDomainDkim(),
"aws_ses_domain_mail_from": resourceAwsSesDomainMailFrom(),
Expand Down
140 changes: 140 additions & 0 deletions aws/resource_aws_ses_domain_identity_policy.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
package aws

import (
"log"

"github.com/hashicorp/terraform/helper/schema"

"fmt"
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/service/ses"
"github.com/hashicorp/terraform/helper/resource"
)

func resourceAwsSesDomainIdentityPolicy() *schema.Resource {
return &schema.Resource{
Create: resourceAwsSesDomainIdentityPolicyCreate,
Read: resourceAwsSesDomainIdentityPolicyRead,
Update: resourceAwsSesDomainIdentityPolicyUpdate,
Delete: resourceAwsSesDomainIdentityPolicyDelete,

Schema: map[string]*schema.Schema{
"arn": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This attribute should be changed to identity or identity_arn to match the expected value. If the API is always setting this to the ARN, we should likely name it identity_arn and validate it via:

ValidateFunc: validateArn,

Type: schema.TypeString,
Required: true,
ForceNew: true,
},
"name": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can validate this argument via:

ValidateFunc: validation.All(
	validation.StringLenBetween(1, 64),
	validation.StringMatch(regexp.MustCompile(`^[a-zA-Z0-9\-\_]$`), "must contain only alphanumeric characters, dashes, and underscores"),
),

Reference: https://docs.aws.amazon.com/ses/latest/APIReference/API_PutIdentityPolicy.html

Type: schema.TypeString,
Required: true,
ForceNew: true,
},
"policy": {
Type: schema.TypeString,
Required: true,
ValidateFunc: validateJsonString,
DiffSuppressFunc: suppressEquivalentAwsPolicyDiffs,
},
},
}
}

func resourceAwsSesDomainIdentityPolicyCreate(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).sesConn

arn := d.Get("arn").(string)
policyName := d.Get("name").(string)
policy := d.Get("policy").(string)

req := ses.PutIdentityPolicyInput{
Identity: aws.String(arn),
PolicyName: aws.String(policyName),
Policy: aws.String(policy),
}

_, err := conn.PutIdentityPolicy(&req)
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should return error context for operators and code maintainers, e.g.

Suggested change
return err
return fmt.Errorf("error creating SES Identity (%s) Policy: %s", identityARN, err)

}

d.SetId(resource.PrefixedUniqueId(fmt.Sprintf("%s-", policyName)))
return resourceAwsSesDomainIdentityPolicyRead(d, meta)
}

func resourceAwsSesDomainIdentityPolicyUpdate(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).sesConn

arn := d.Get("arn").(string)
policyName := d.Get("name").(string)
policy := d.Get("policy").(string)

req := ses.PutIdentityPolicyInput{
Identity: aws.String(arn),
PolicyName: aws.String(policyName),
Policy: aws.String(policy),
}

_, err := conn.PutIdentityPolicy(&req)
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should return error context here for operators and code maintainers:

Suggested change
return err
return fmt.Errorf("error updating SES Identity (%s) Policy (%s): %s", identityARN, policyName, err)

}

return resourceAwsSesDomainIdentityPolicyRead(d, meta)
}

func resourceAwsSesDomainIdentityPolicyRead(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).sesConn

arn := d.Get("arn").(string)
policyName := d.Get("name").(string)
policyNames := make([]*string, 1)
policyNames[0] = aws.String(policyName)

policiesOutput, err := conn.GetIdentityPolicies(&ses.GetIdentityPoliciesInput{
Identity: aws.String(arn),
PolicyNames: policyNames,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: The above logic can be simplified with:

Suggested change
PolicyNames: policyNames,
PolicyNames: aws.StringSlice([]string{policyName}),

})
if err != nil {
if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "NotFound" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic can be simplified with a helper function we have available:

Suggested change
if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "NotFound" {
if isAWSErr(err, "NotFound", "") {

log.Printf("[WARN] SES Domain Identity Policy (%s) not found, error code (404)", policyName)
d.SetId("")
return nil
}

return err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should return error context here for operators and code maintainers:

Suggested change
return err
return fmt.Errorf("error reading SES Identity (%s) Policy (%s): %s", identityARN, policyName, err)

}

if policiesOutput.Policies == nil {
log.Printf("[WARN] SES Domain Identity Policy (%s) not found (nil)", policyName)
d.SetId("")
return nil
}
policies := policiesOutput.Policies

policy, ok := policies[*aws.String(policyName)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The casting to a pointer and dereferencing looks extraneous here:

Suggested change
policy, ok := policies[*aws.String(policyName)]
policy, ok := policies[policyName]

if !ok {
log.Printf("[WARN] SES Domain Identity Policy (%s) not found in attributes", policyName)
d.SetId("")
return nil
}

d.Set("policy", policy)
return nil
}

func resourceAwsSesDomainIdentityPolicyDelete(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).sesConn

arn := d.Get("arn").(string)
policyName := d.Get("name").(string)

req := ses.DeleteIdentityPolicyInput{
Identity: aws.String(arn),
PolicyName: aws.String(policyName),
}

log.Printf("[DEBUG] Deleting SES Domain Identity Policy: %s", req)
_, err := conn.DeleteIdentityPolicy(&req)
return err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should return error context here for operators and code maintainers:

Suggested change
return err
if err != nil {
return fmt.Errorf("error deleting SES Identity (%s) Policy (%s): %s", identityARN, policyName, err)
}
return nil

}
58 changes: 58 additions & 0 deletions aws/resource_aws_ses_domain_identity_policy_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package aws

import (
"regexp"
"testing"

"fmt"
"github.com/hashicorp/terraform/helper/acctest"
"github.com/hashicorp/terraform/helper/resource"
)

func TestAccAWSSESDomainIdentityPolicy_basic(t *testing.T) {
domain := fmt.Sprintf(
"%s.terraformtesting.com.",
acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum))

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAwsSESDomainIdentityDestroy,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new CheckDestroy function should be created that verifies from the API that all SES Identity Policy configured in the test configurations are deleted. See also: https://www.terraform.io/docs/extend/testing/acceptance-tests/testcase.html#checkdestroy

Steps: []resource.TestStep{
{
Config: testAccAWSSESDomainIdentityConfig_withPolicy(domain),
Check: resource.ComposeTestCheckFunc(
testAccCheckAwsSESDomainIdentityExists("aws_ses_domain_identity.test"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new testAccCheckAwsSESIdentityPolicyExists() check function should be created that verifies from the API that the SES Identity Policy referenced by the given resource name was in fact properly created.

resource.TestMatchResourceAttr("aws_ses_domain_identity_policy.custom", "policy",
regexp.MustCompile("^{\"Version\":\"2012-10-17\".+")),
),
},
},
})
}

func testAccAWSSESDomainIdentityConfig_withPolicy(domain string) string {
return fmt.Sprintf(`
resource "aws_ses_domain_identity" "test" {
name = "%s"
}

resource "aws_ses_domain_identity_policy" "custom" {
arn = "${aws_ses_domain_identity.test.arn}"
name = "test"
policy = <<POLICY
{
"Version":"2012-10-17",
"Id": "default",
"Statement":[{
"Sid":"default",
"Effect":"Allow",
"Principal":{"AWS":"*"},
"Action":["SES:SendEmail","SES:SendRawEmail"],
"Resource":"${aws_ses_domain_identity.test.arn}"
}]
}
POLICY
}
`, domain)
}