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

Add cname variable for static-website construct #92

Merged
merged 2 commits into from
Sep 3, 2021

Conversation

fredericbarthelet
Copy link
Collaborator

When provisioning Route53 records using Cloudformation, there is no way to reference distribution CNAME generated by CloudFront (even while using custom domain names).

resources:
  Resources:
    Route53Record:
      Type: AWS::Route53::RecordSet
      Properties:
        HostedZoneId: XXXXX
        Name: app.mydomain.
        Type: A
        AliasTarget:
          HostedZoneId: XXXXX
          DNSName: !GetAtt ???.DomainName

This PR adds the following variable on the static-website construct :

  • cname: the domain name of the resource, such as d111111abcdef8.cloudfront.net

@t-richard
Copy link
Contributor

While having the cname as a variable makes sense, wouldn't it be better for this use case to have route 53 support inside the construct that would add a record to the hosted zone for each configured domain if a hostedZone parameter is provided?

docs/static-website.md Outdated Show resolved Hide resolved
@BaptistG
Copy link

BaptistG commented Sep 1, 2021

@t-richard Indeed it would be even better if the plugin could handle creating the records when hostedZone is provided.

If hostedZone is provided the plugin could even handle certificate creation/validation (would have to be created in us-east-1).

However I don't know how much work would be needed to implement these features and the current state of the PR fixes the main issue which is creating DNS records for Cloudfront distributions created by the plugin.

@t-richard
Copy link
Contributor

the plugin could even handle certificate creation/validation (would have to be created in us-east-1).

The last part is the main problem. It would require creating and managing a whole new, separate stack just for the certificate and even then, manual action would be required to validate the certificate by DNS or email.

Whereas the route 53 part is quite easier if you have the hosted zone id.

@fredericbarthelet
Copy link
Collaborator Author

fredericbarthelet commented Sep 1, 2021

The last part is the main problem. It would require creating and managing a whole new, separate stack just for the certificate and even then, manual action would be required to validate the certificate by DNS or email.

Just checked with an AWS DA, you can actually bypass this problem using a custom resource from CloudFormation, and provision the certificate using the SDK. It's not plain simple, but it does the job and is apparently widely used to solve this kind of issue. The CDK offers the ability to easily implement such custom resource, but it relies on having a bootstrap stack to provision corresponding lambda associated with the custom resource, which is not currently done in Lift.

As suggested @BaptistG, merging as is (with the modification of the documentation) enables people to reference variable from the construct without impacting futur evolution of adding Route53 or not. I suggest we go ahead with this variable exposition and start discussion on bringing Routee53 in the construct in another PR.

@mnapoli mnapoli added the enhancement New feature or request label Sep 2, 2021
Copy link
Member

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

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

As suggested @BaptistG, merging as is (with the modification of the documentation) enables people to reference variable from the construct without impacting futur evolution of adding Route53 or not. I suggest we go ahead with this variable exposition and start discussion on bringing Routee53 in the construct in another PR.

100% agree, it's a good small improvement, we can make it better in an another step.

The "HostedZoneId" comment is a good idea to change in the PR, and except that LGTM to merge and release!

Copy link
Contributor

@t-richard t-richard left a comment

Choose a reason for hiding this comment

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

Expect this minor comment and the one from @BaptistG, it looks good to me !

docs/static-website.md Outdated Show resolved Hide resolved
@t-richard t-richard mentioned this pull request Sep 3, 2021
9 tasks
@fredericbarthelet fredericbarthelet merged commit b06714f into master Sep 3, 2021
@fredericbarthelet fredericbarthelet deleted the export-cloudfront-domain branch September 3, 2021 07:01
@t-richard
Copy link
Contributor

I think for auto-setup on Route 53, we can wait for #44 to be merged and implement it for both at the same time.

The CDK offers the ability to easily implement such custom resource, but it relies on having a bootstrap stack to provision corresponding lambda associated with the custom resource, which is not currently done in Lift.

CDK is filling the gap on that side (and other things like lookups, multi-stacks deploy, etc) but those require setting up an environment and sometimes this boostrap stack. This is clearly explained when using the CDK directly but might be less obvious when the CDK is shadowed like in Lift.

@mnapoli
Copy link
Member

mnapoli commented Sep 3, 2021

Yeah, we can't really use CDK's custom resources because of the main flaw of Lift right now: we don't use the CDK's bootstrap stack. That means that we can't deploy any Lambda function with CDK.

Else is there any way we can do this more interactively? I've added a suggestion in #84 but any other idea is welcome! (and I think we should continue the discussion there)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants