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
167 stars 13 forks source link

fix: Specify FQDN of Cognito login to Route53 #85

Closed kc0bfv closed 6 months ago

kc0bfv commented 6 months ago

This changes the Route53 A Record for Cognito's login page to specify the FQDN of the login page, instead of a relative version. That's important if you're in the situation where your Route53 base is something like example.com, but your analytics domain is analytics.example.com, because in that case your Cognito login needs to be login.analytics.example.com.

Prior to this patch the domain was going to Route53 A Record as just "login", in that example, and so the record that was built was for login.example.com (relative to R53 base). Instead, now, it'll build login.analytics.example.com.

Note that Route53 FQDN spec needs a recordName with a trailing dot (an actual FQDN). The Cognito customDomain piece actually gets screwed up if it has that trailing dot. Surprising.

I think I was having the same issue as https://github.com/rehanvdm/serverless-website-analytics/issues/84, and I think this resolves it.

rehanvdm commented 6 months ago

Thanks for the contribution. So I understand now what the logic is for the recordName field. If we hav a look in the CDK code here:

We see that there is no differnce in specifying it as:

login

Or

login.analytics.example.com.

If it ends in . it will be used as is, if it does not have the domain (so if only login) then it will append and the domain and . so it becomes login.analytics.example.com. in the CFN template.

So this change is not breaking. I mentioned in that issue https://github.com/rehanvdm/serverless-website-analytics/issues/84#issuecomment-2041046059 why it flew under my radar, but the TL;DR is that I assmed everyone will create a new HostedZone for there subdomain, then it works, but if you don't then that bug happens.

github-actions[bot] commented 6 months ago

:tada: This PR is included in version 1.7.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

kc0bfv commented 6 months ago

Honestly I should have kept it simple and just created a new zone, like you suggest. But 🤷‍♂️

I like this tool - I appreciate how easy it is to deploy serverless, how cheap it is to operate, and how it respects user privacy. For my super simple use cases it's a great alternative to Goog Analytics - I started writing something like this myself, but got wrapped up in how to make the whole thing operate as cheaply as possible.

Thanks for building this!

rehanvdm commented 6 months ago

🙂 Pleasure, and thanks for your contribution.