-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(ses): https policy for custom tracking domain #34314
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
Conversation
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.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
@@ -189,6 +216,9 @@ export class ConfigurationSet extends Resource implements IConfigurationSet { | |||
throw new ValidationError(`The maximum delivery duration must be less than or equal to 14 hours (50400 seconds), got: ${props.maxDeliveryDuration.toSeconds()} seconds.`, this); | |||
} | |||
} | |||
if (props.customTrackingHttpsPolicy && !props.customTrackingRedirectDomain) { |
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.
Cloudformation causes deployment error in this condition.
code
new ses.ConfigurationSet(this, 'ConfigurationSet', {
// customTrackingRedirectDomain: 'tracking.naonao.com',
customTrackingHttpsPolicy: ses.HttpsPolicy.OPTIONAL,
});
result
Resource handler returned message: "1 validation error detected: Value at 'trackingOptions.customRedirectDomain' failed to satisfy constraint: Member must not be null (Service: SesV2, Status Code: 400, Request ID: 866fb1c3-dd63-454f-94ec-2ca1bc7b7b1d)" (RequestToken: 05ae07af-ba64-77f8-0cbc-da45a39b8d5f, HandlerErrorCode: InvalidRequest)
/** | ||
* The https policy to use for tracking open and click events. | ||
* | ||
* @default - HttpsPolicy.OPTIONAL if customTrackingRedirectDomain is set, otherwise undefined |
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.
SES default behavior is described in here.
Optional – (Default behavior) Open tracking links will be wrapped using HTTP.
https://docs.aws.amazon.com/ses/latest/dg/creating-configuration-sets.html
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.
Looks good. Left very minor comments. But I can approve it as is if it's better the way it is :)
new integ.IntegTest(app, 'ConfigurationSetInteg', { | ||
testCases: [new ConfigurationSetStack(app, 'ses-configuration-set-tracking-options-integ', { | ||
hostedZoneId, | ||
hostedZoneName, | ||
domainName, | ||
})], | ||
}); |
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.
How about specifying enableLookups: true
and stackUpdateWorkflow: false
options just in case we forget --disable-update-workflow
for the integ command?
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 don't know that these options have the same meanings. Thanks!
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.
Sorry, I misunderstood. The fromHostedZoneAttributes
does not look up, so the enableLookups: true
was not needed. Could you please remove it?
interface TestStackProps extends StackProps { | ||
hostedZoneId: string; | ||
hostedZoneName: string; | ||
domainName: string; |
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.
Isn't the domainName
unnecessary?
"EmailIdentity7187767D": { | ||
"Type": "AWS::SES::EmailIdentity", | ||
"Properties": { | ||
"EmailIdentity": "naonao.org" |
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.
Is it okay if you don't keep it hidden?
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 thought there was no particular problem, but just to be safe, I'll use example.com
!
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 thought that in CI, the domain name would always be example.com
, but why were the tests passing without any problems..??
I noticed that this value is derived from not CDK_INTEG_DOMAIN_NAME but CDK_INTEG_HOSTED_ZONE_NAME. But I still don't understand why this integ test had passed. I'll wait for the CI result.
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 see, I missed it.
new integ.IntegTest(app, 'ConfigurationSetInteg', {
testCases: [new ConfigurationSetStack(app, 'ses-configuration-set-tracking-options-integ', {
hostedZoneId,
hostedZoneName,
})],
Two stacks must be explicitly specified for testCases
. The stack not specified would not be checked.
This means that we must generate the two stacks separately, define their dependencies with , and specify the stacks in addDependency
testCases
.
Also, you now specify ConfigurationSetStack for the scope of IdentityStack, but need to specify app
instead.
P.S.
define their dependencies with
addDependency
The value originally generated by IdentityStack is used by ConfigurationSetStack, so no explicit dependency is needed.
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.
However, if you make it a target of snapshot verification, this test can only be performed by you who own that domain in the future.
For this reason, we would need to either exclude the stack from the targets, or prompt developers to rewrite the generated template. (If it is good and safe to detect changes to the stack by other PRs about the EmailIdentity
too, would the latter be a good idea...? like: https://github.com/aws/aws-cdk/blob/v2.195.0/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-ecr-build-and-publish-private.ts#L15)
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.
Actually, since domain verification on EmailIdentity
takes several dozen seconds, when defined as two stacks with dependencies on the same app, the ConfigurationSetStack
deployment begins before authentication is complete, resulting in a deployment error. At first, I had created this integ test like that.
As a workaround, I've made it a nested stack to significantly delay the start of ConfigurationSetStack
deployment, ensuring that deployment executes after domain verification is complete.
(Therefore, I think this test deployment could be flaky and may fail in some cases.)
I'm wondering if it might be better to simply create EmailIdentity
in advance and only test ConfigurationSetStack
.
What do you think?
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'm wondering if it might be better to simply create EmailIdentity in advance and only test ConfigurationSetStack.
Agree with it! There should be no need to make this in CDK.
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 updated integ test like that!
@go-to-k Thank you for your review!! I've addressed your comments. |
update domain name
84fe08d
to
6db3e24
Compare
* | ||
* Step 1: Create a public hosted zone in Route53 | ||
* Step 2: Create a email identity in SES and validate it with the hosted zone | ||
* Step 3: Set the domain name as an env var "DOMAIN_NAME" |
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.
const domainName = process.env.CDK_INTEG_DOMAIN_NAME ?? process.env.DOMAIN_NAME;
In this case, CDK_INTEG_DOMAIN_NAME
is set to '*.example.com' in integ-runner.
At this time, even if we set DOMAIN_NAME
, it won't be reflected?
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.
CDK_INTEG_DOMAIN_NAME
is only set in CI environment and I could execute integ test with my domain by setting DOMAIN_NAME
. (But I don't know why the CDK_INTEG_DOMAIN_NAME
env is not set during local integ test execution.)
This implementation is used in other integ tests.
Additionally, I need to add to the procedure that the domain name must be corrected to example.com
in the generated CloudFormation template.
if (!hostedZoneId) throw new Error('For this test you must provide your own HostedZoneId as an env var "HOSTED_ZONE_ID". See framework-integ/README.md for details.'); | ||
const hostedZoneName = process.env.CDK_INTEG_HOSTED_ZONE_NAME ?? process.env.HOSTED_ZONE_NAME; | ||
if (!hostedZoneName) throw new Error('For this test you must provide your own HostedZoneName as an env var "HOSTED_ZONE_NAME". See framework-integ/README.md for details.'); | ||
const domainName = process.env.CDK_INTEG_DOMAIN_NAME ?? process.env.DOMAIN_NAME; |
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.
Isn't it CDK_INTEG_HOSTED_ZONE_NAME
(example.com
) instead of CDK_INTEG_DOMAIN_NAME
(*.example.com
)?
related: https://github.com/aws/aws-cdk/pull/34314/files#r2086912984
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.
You are exactly right! I'll fix this.
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.
Thanks for the changes, LGTM!
@go-to-k Thanks always! |
testCases: [new ConfigurationSetStack(app, 'ses-configuration-set-tracking-options-integ', { | ||
hostedZoneName, | ||
})], | ||
stackUpdateWorkflow: false, |
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.
wondering why disable the update workflow?
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.
This is described in here and I don't know why this is required..
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 see. Thanks for the pointer. Make sense to skip the update workflow as the fake hosted zone name will not deploy successfully.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
LGTM
testCases: [new ConfigurationSetStack(app, 'ses-configuration-set-tracking-options-integ', { | ||
hostedZoneName, | ||
})], | ||
stackUpdateWorkflow: false, |
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 see. Thanks for the pointer. Make sense to skip the update workflow as the fake hosted zone name will not deploy successfully.
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
None
Reason for this change
Cloudformation supports for configuring HttpsPolicy for custom tracking domain in
ConfigurationSet
but AWS CDK cannot do this.https://docs.aws.amazon.com/ja_jp/AWSCloudFormation/latest/UserGuide/aws-properties-ses-configurationset-trackingoptions.html#cfn-ses-configurationset-trackingoptions-httpspolicy
Description of changes
HttpsPolicy
enumcustomTrackingHttpsPolicy
toConfigurationSetProps
Describe any new or updated permissions being added
None
Description of how you validated changes
Add both unit and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license