[WIP] new resource for ses domain identity Policy #5128
Conversation
bflad
added
new-resource
|
I'm not sure, why this merge request is stale for the last 5 month. My guess is, it's because of the |
|
@gmyers-amfam Nice work, thanks. We want to use this resource too. How can we unblock the merging step? Who can approve this PR? I guess that removing the "WIP" word from the title (if it's ready) and ping an approver could help. Thanks in advance. |
|
@gmyers-amfam Looking at the code i can make a wild guess what the blocking issues are: It seems that there is no documentation added in this pull-request. Since this change introduces a new resource there should probably be added a documentation page on the website with this pull-request. Also, i can imagine this pull-request becomes easier for the maintainers to merge if there were some tests included. So lets first add some docs and tests, next remove the [wip] and lastly ping the maintainers. |
|
Hi, any updates? We are terraforming SES and we have SES domain identity policies. Thanks |
There was a problem hiding this comment.
Hi @gmyers-amfam 👋 Thanks for submitting this. See the below for initial feedback. Please reach out if you have any questions or do not have time to implement the items.
| Delete: resourceAwsSesDomainIdentityPolicyDelete, | ||
|
|
||
| Schema: map[string]*schema.Schema{ | ||
| "arn": { |
There was a problem hiding this comment.
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,| Required: true, | ||
| ForceNew: true, | ||
| }, | ||
| "name": { |
There was a problem hiding this comment.
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
|
|
||
| _, err := conn.PutIdentityPolicy(&req) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
We should return error context for operators and code maintainers, e.g.
| return err | |
| return fmt.Errorf("error creating SES Identity (%s) Policy: %s", identityARN, err) |
|
|
||
| _, err := conn.PutIdentityPolicy(&req) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
We should return error context here for operators and code maintainers:
| return err | |
| return fmt.Errorf("error updating SES Identity (%s) Policy (%s): %s", identityARN, policyName, err) |
|
|
||
| policiesOutput, err := conn.GetIdentityPolicies(&ses.GetIdentityPoliciesInput{ | ||
| Identity: aws.String(arn), | ||
| PolicyNames: policyNames, |
There was a problem hiding this comment.
Nit: The above logic can be simplified with:
| PolicyNames: policyNames, | |
| PolicyNames: aws.StringSlice([]string{policyName}), |
| return nil | ||
| } | ||
|
|
||
| return err |
There was a problem hiding this comment.
We should return error context here for operators and code maintainers:
| return err | |
| return fmt.Errorf("error reading SES Identity (%s) Policy (%s): %s", identityARN, policyName, err) |
|
|
||
| log.Printf("[DEBUG] Deleting SES Domain Identity Policy: %s", req) | ||
| _, err := conn.DeleteIdentityPolicy(&req) | ||
| return err |
There was a problem hiding this comment.
We should return error context here for operators and code maintainers:
| return err | |
| if err != nil { | |
| return fmt.Errorf("error deleting SES Identity (%s) Policy (%s): %s", identityARN, policyName, err) | |
| } | |
| return nil |
| resource.Test(t, resource.TestCase{ | ||
| PreCheck: func() { testAccPreCheck(t) }, | ||
| Providers: testAccProviders, | ||
| CheckDestroy: testAccCheckAwsSESDomainIdentityDestroy, |
There was a problem hiding this comment.
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
| { | ||
| Config: testAccAWSSESDomainIdentityConfig_withPolicy(domain), | ||
| Check: resource.ComposeTestCheckFunc( | ||
| testAccCheckAwsSESDomainIdentityExists("aws_ses_domain_identity.test"), |
There was a problem hiding this comment.
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.
| @@ -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(), | |||
There was a problem hiding this comment.
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_policyas 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.markdownfile and a website sidebar link inwebsite/aws.erb
bflad
added
the
waiting-response
|
Any update regarding this ? That would be very useful ! |
ghost
removed
the
waiting-response
bflad
added
the
waiting-response
|
Since we have not heard anything from the original author regarding this, the implementation has been finished in a followup commit. The new resource is named |
Reference: #5128 (review) Output from acceptance testing: ``` --- PASS: TestAccAWSSESIdentityPolicy_Identity_Email (13.57s) --- PASS: TestAccAWSSESIdentityPolicy_basic (13.92s) --- PASS: TestAccAWSSESIdentityPolicy_Policy (20.09s) ```
|
This has been released in version 2.16.0 of the Terraform AWS provider. 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! |
ghost
locked and limited conversation to collaborators
ghost
removed
the
waiting-response
Changes proposed in this pull request:
Output from acceptance testing:
This is my first Terraform pull request and I'm a novice when it comes to Go. I tried running the
make testacccommand above but got a lot of "cannot find package" errors. Is anyone able to assist in either getting me up to speed, or getting this PR polished and tested?A note on this resource: SES Domain Identities can have any number of policies attached to them, similar to how an S3 bucket has a policy. SES Domain Identity Policies allow for things like cross-account sending access (by setting the 'Principals' field in the policy accordingly) which is clearly missing from Terraform at the moment, hence this PR.