rehanvdm / serverless-website-analytics

A CDK construct that consists of a serverless backend, frontend and client side code to track website analytics
GNU General Public License v2.0
166 stars 13 forks source link

Certificate defined within the stack doesn't have a region so fails construct validation #34

Closed tomharvey closed 1 year ago

tomharvey commented 1 year ago

I am defining the HostedZone and certificate within my stack, so, the region check is failing because it gets a TOKEN instead of us-east-1:

Error: Certificate must be in us-east-1, not in ${Token[TOKEN.646]}, this is a requirement for CloudFront

Is having the cert (and hosted zone) in a separate stack such a strong best practice that the construct shouldn't support it?

If you want to allow this, then when a TOKEN is received, you could check the region of the stack. Alternatively, provide some prop to disable the region checking.

Stack looks like this:

import * as cdk from 'aws-cdk-lib';
import { Construct } from 'constructs';
import { Swa, SwaProps } from 'serverless-website-analytics'

export class WebAnalyticsStack extends cdk.Stack {
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const analyticsZone = new cdk.aws_route53.PublicHostedZone(this, 'HostedZone', {
      zoneName: 'analytics.example.com',
    });

    const certificate = new cdk.aws_certificatemanager.Certificate(this, 'Certificate', {
      domainName: 'analytics.example.com',
      subjectAlternativeNames: ["*.analytics.example.com"],
      validation: cdk.aws_certificatemanager.CertificateValidation.fromDns(analyticsZone),
    });

    const options: SwaProps = {
      environment: 'production',
      awsEnv: {
        account: this.account,
        region: this.region
      },
      sites: ['example.com'],
      domain: {
        name: 'web.analytics.example.com',
        certificate: certificate,
        hostedZone: analyticsZone,
      },
      allowedOrigins: ["*"]
    }

    new Swa(this, 'web-analytics', options)
  }
}
rehanvdm commented 1 year ago

In what region are you deploying the stack to? That error comes from over here. It seems that I placed this here so that it errors before the CFN (CloudFront) deployment starts and then fails with a s similar error.

Is having the cert (and hosted zone) in a separate stack such a strong best practice that the construct shouldn't support it?

It was because I knew that people would want to deploy the serverless-website-analytics construct to any region and CloudFront imposes the restriction that the cert must be in us-east-1 therefore I did not want to make it the responsibility of the construct and also be restricted by it.

If you want to allow this, then when a TOKEN is received, you could check the region of the stack. Alternatively, provide some prop to disable the region checking.

In the link above I check the region of cert only as that is the important part. I do not want to disable this as I think it is good to check before the CFN deployment.

rehanvdm commented 1 year ago

In the test/example stack that is the demo site, I import both the hosted zone and cert because of this restriction.

But you can define another stack in your app that is in us-east-1 and define the Cert in there. Then you need to export the value by means of SSM parameters because CFN output values only work if the stacks are in the same region.

I just learned of the new crossregionreferences option that you can add to the stacks and then the CDK would export and import it via SSM when necessary it seems. Otherwise you would have to DIY, here is a quick example I found from SO and a blog from AWS itself.

tomharvey commented 1 year ago

I am deploying to us-east-1 because I know it's an important region for ACM and CloudFront. I've been burned by that before, so, I totally get why you'd want to throw an exception before we wait the 20-ish minutes for CloudFront to build!

Completely get why you built it like that and why you'd want to protect from deployment to the wrong region

I've moved to building the Zone and ACM in a separate stack and using SSM as you suggest.

If you want to keep the protection from mistaken regions, I get that. But wanted to run it up the flag pole, so to speak, see if you're certain that you want to block the usage of the Zone and Cert being in the same stack.

rehanvdm commented 1 year ago

Cool glad to hear you got it working.

So just to double-check, it threw that error when you deployed the stack as in your example to us-east-1? With your cert, zone and serverless-website-analytics constructs all in us-east-1? Because if that is the case then it is a bug, it means this if condition is failing me for some reason.

tomharvey commented 1 year ago

Yes. It threw that error when I deployed the stack I pasted above to us-east-1.

The if condition is failing because it's being checked before the certificate region can be resolved - see https://docs.aws.amazon.com/cdk/v2/guide/tokens.html

Just for clarity, I was creating the stack in stages (first, define the zone and deploy, then the cert and deploy, then the web analytics and deploy), so the cert was definitely created and validated. But, at synth time, even created resources in a stack aren't fully resolved.

That's why I'm not sure that there is a great solution to move forward. As far as I can see it's either:

  1. Pass a prop to turn off the check
  2. When the if condition gets TOKEN instead of a valid region string, read this.region from the construct to check we're deploying the whole thing to us-east-1

But, very open to ideas. Maybe take to the cdk.dev slack?

rehanvdm commented 1 year ago

Yeah I'm there :) feel free to ping me (Rehan van der Merwe) but I think I see what is happening. In my testing I only ever imported the cert and because of that the way I got the region always worked. When the cert is in the same stack I would have to use .resolve to get the actual value of the token which I can then compare for the region.

tomharvey commented 1 year ago

If you did want to maintain that the zone and cert should be in a separate stack, you could (in a future, this might be a API breaking change) move toward accepting certificateArn: string and hostedzoneId: string as props instead of certificate: ICertificate and hostedZone: iHostedZone. Then call Certificate.fromCertificateArn and HostedZone.fromHostedZoneAttributes within the construct using those string props.

That then becomes an explicit way of telling me and people like me not to mix the zone and cert in with the rest of the stack.

Having everything in one stack, deploying the HostedZone, the Certificate and the Swa in one pass has some timing issues around getting the NS records setup and propagated. So, maybe splitting should be a "strong best practice that the construct shouldn't support" as I asked initially.

rehanvdm commented 1 year ago

Ahh okay, I hear you. But passing string ARN's for the sake of validation just doesn't feel right and I don't want to force anyone to create it a different stack, if they try to do it like you did then it should work.

I would rather for now remove my pre-check. I think in the future I would rename the property from certificate to usEast1Certificate to be even more explicit about it. Then if it fails at CFN deploy time, they will search for the error and probably land on this discussion :)

I see I didn't call it out explicitly in the docs. So to summarize, I think I will:

FYI, that pre-check was quite naive of me to expect the ARN to have resolved with a concrete value. Even if I did:


const stack = Stack.of(scope)
const certArn = stack.resolve(props.domain.certificate.certificateArn)

It still resovles to a Ref value. So indeed the only way would be to pass a string explicitly as you mentioned. So I will make the three changes above.

Thanks for bringing it to my attention :)

tomharvey commented 1 year ago

Sounds like a great way forward.

Loving the project! 🙏🏻👏🏻