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

feat(sl): enhance site launch process to utilise DNS indirection layer #920

Conversation

dcshzj
Copy link
Contributor

@dcshzj dcshzj commented Aug 24, 2023

Problem

Closes IS-498.

Solution

Breaking Changes

  • Yes - this PR contains breaking changes
  • No - this PR is backwards compatible

Improvements:

  • The site launch process has been enhanced to introduce a new DNS indirection layer for Amplify-hosted domains. The full details of this DNS indirection layer is available in this RFC. More specifically for this PR:
    • A new database column in the launches table is introduced to store the indirection domains. For backward compatibility, the value in this column can be NULL, but new site launches must have this indirection domain.
    • A new step has been added after the CMS creates the domain association on Amplify, but before the record is created in DynamoDB to create the indirection domain on the isomer-indirection repo.
    • The process for ProdOps will be similar to how it is done for creating redirections, which is to open a pull request on the isomer-indirection repo from staging to main, getting someone else to approve it and merging the pull request. The records will be automatically created on Route 53 using Pulumi.
    • The email sent to ProdOps with the DNS records has also been updated to use the indirection domain as the DNS record value, instead of the primary domain target (i.e. Cloudfront) previously.

Tests

The full site launch process will need to be executed. The tentative plan is to test this with a single site launch happening on the week of 28 August.

Deploy Notes

@dcshzj dcshzj requested a review from a team August 24, 2023 00:37
Copy link
Contributor

@kishore03109 kishore03109 left a comment

Choose a reason for hiding this comment

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

Code mostly lgtm, what is our plan looking like moving forward? is it after every launch ops would have to merge the pr?
should we do a if SL success -> merge pr , if SL failure -> delete corresponding repo? that way we elegantly handle this case + dont have to add additional process to ops?

src/config/config.ts Outdated Show resolved Hide resolved
@dcshzj
Copy link
Contributor Author

dcshzj commented Aug 24, 2023

@kishore03109 Can I check for the existing redirection repo logic, do/should we also revert the change on site launch failure?

@kishore03109
Copy link
Contributor

Can I check for the existing redirection repo logic, do/should we also revert the change on site launch failure?
wah hmm you raise a fair point. it makes sense for now to do so?

@dcshzj
Copy link
Contributor Author

dcshzj commented Aug 25, 2023

@kishore03109 I am deprioritising the reverting on site launch failure for now as I think the redirection and indirection stuff currently still has a manual component so we can easily catch and recover. Plus, the logic for creating the indirection DNS record also handles the case where the file already exists and will update the values inside.

I am thinking in the grander scheme of things, it is very unlikely that the agency will abort a site launch completely due to the technical errors we face when launching sites. Ultimately, we will retry the site launch again in the future and with our existing mechanisms in place to handle a potential existing file, I think reverting back to the original state is not as important.

@dcshzj dcshzj requested a review from kishore03109 August 25, 2023 00:37
@kishore03109
Copy link
Contributor

@kishore03109 I am deprioritising the reverting on site launch failure for now as I think the redirection and indirection stuff currently still has a manual component so we can easily catch and recover. Plus, the logic for creating the indirection DNS record also handles the case where the file already exists and will update the values inside.
I am thinking in the grander scheme of things, it is very unlikely that the agency will abort a site launch completely due to the technical errors we face when launching sites. Ultimately, we will retry the site launch again in the future and with our existing mechanisms in place to handle a potential existing file, I think reverting back to the original state is not as important.

@dcshzj ok makes sense, we can go with this flow for now

Copy link
Contributor

@kishore03109 kishore03109 left a comment

Choose a reason for hiding this comment

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

I am ok with the code changes, but with anything to do with Infra, I am always abit nervous about the integration.

Before you merge into master, can I trouble you to test the full flow on staging for one test site?

happy to huddle for this, and I think is impt before release as it affects the formsg site launch feature too!

@dcshzj
Copy link
Contributor Author

dcshzj commented Aug 28, 2023

Tested successfully on staging. The records were sent successfully via email:

Screenshot 2023-08-28 at 10 38 13 AM

The DNS records were also correctly set up on the isomer-indirections repository (see PR #12 and the Pulumi deployment).

@dcshzj dcshzj merged commit a00eb2e into develop Aug 29, 2023
@mergify mergify bot deleted the IS-498-enhance-site-launch-process-to-utilise-dns-indirection-layer branch August 29, 2023 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants