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 dns work flows #274

Merged
merged 1 commit into from
Mar 1, 2023
Merged

Add dns work flows #274

merged 1 commit into from
Mar 1, 2023

Conversation

Genne23v
Copy link
Contributor

@Genne23v Genne23v commented Feb 24, 2023

Close #217 and parts of #270

This PR adds workflows as below.

  1. Create a record in DB with PENDING status. Return id to update status later.
  2. Create a record in Route53 (TTL property added due to error)
  3. Poll Route53 in every second to get the updated status (INSYNC or something else)
  4. Update the DB either with active or error

To test this, it needs manual steps as below.

  • Create a hostedZone using unit test dns.server.test.ts. If you run all test, it will remove hostedZone. Run partial test.
  • Get hostedZoneId from localhost:5053/moto-api and set AWS_ROUTE53_HOSTED_ZONE_ID
  • Set ROOT_DOMAIN to starchart.com
  • Go to http://localhost:8080/dev and hit Request DNS Record button

@Genne23v Genne23v requested review from a user and humphd February 24, 2023 02:07
@SerpentBytes SerpentBytes self-requested a review February 24, 2023 02:13
Copy link
Contributor

@SerpentBytes SerpentBytes left a comment

Choose a reason for hiding this comment

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

@Genne23v E2E tests are failing because WS_ROUTE53_HOSTED_ZONE_ID is missing.

@Genne23v
Copy link
Contributor Author

Genne23v commented Feb 24, 2023

@Genne23v E2E tests are failing because WS_ROUTE53_HOSTED_ZONE_ID is missing.

It shouldn't throw in testing environment. I will take a closer look tomorrow.

app/lib/dns.server.ts Outdated Show resolved Hide resolved
app/lib/dns.server.ts Outdated Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
app/queues/dns/dns-flow.server.ts Outdated Show resolved Hide resolved
@humphd
Copy link
Contributor

humphd commented Feb 24, 2023

I filed #275 to improve the situation for local dev and needing a hosted zone in moto.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Agree with @dadolhay's comments, and added some more of my own.

app/queues/dns/create-record-worker.server.ts Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
@humphd
Copy link
Contributor

humphd commented Feb 24, 2023

Another thought I have while reading this. We're starting to add a lot of queues. Each queue requires more memory, Redis event handlers, etc. At some point, we'll have to measure. I know in the official docs, the BullMQ talks about queues not being a huge drain.

Related to the above, I also wonder if we should use the same queue or worker for all common tasks. For example: all Route53 operations are going to get made against the same API, and share the same rate limiting. We can set a rate limit on a worker to avoid hitting this.

From https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/DNSLimitations.html, here are the Route53 API limits we need to respect:

All Amazon Route 53 API requests
For the Amazon Route 53 APIs five requests per second per AWS account. If you submit more than five requests per second, Amazon Route 53 returns an HTTP 400 error (Bad request). The response header also includes a Code element with a value of Throttling and a Message element with a value of Rate exceeded.

Note
If your application exceeds this limit, we recommend that you implement exponential backoff for retries. For more information, see Error Retries and Exponential Backoff in AWS in the Amazon Web Services General Reference.

ChangeResourceRecordSets requests
If Route 53 can't process a request before the next request arrives, it will reject subsequent requests for the same hosted zone and return an HTTP 400 error (Bad request). The response header also includes a Code element with a value of PriorRequestNotComplete and a Message element with a value of The request was rejected because Route 53 was still processing a prior request.

@Genne23v
Copy link
Contributor Author

Thank you for the issue. I will assign myself and work as a next step.

@Genne23v
Copy link
Contributor Author

Genne23v commented Feb 24, 2023

Another thought I have while reading this. We're starting to add a lot of queues. Each queue requires more memory, Redis event handlers, etc. At some point, we'll have to measure. I know in the official docs, the BullMQ talks about queues not being a huge drain.

I will take a look at this and write an issue for follow-up. Thank you

@Genne23v Genne23v requested review from a user, SerpentBytes and humphd February 25, 2023 00:50
@Genne23v Genne23v added category: back end Back end part of our web service category: DNS A service about hosting domains category: queue A service that connects and manages certificate creation/expiration category: data Anything related to data management and structure labels Feb 25, 2023
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

I think this looks like something we could take. I've made a few more comments, but in general, we could start with this and improve going forward.

app/queues/dns/create-record-worker.server.ts Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
ghost
ghost previously approved these changes Feb 27, 2023
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks nice!

@Genne23v
Copy link
Contributor Author

Looks nice!

Thank you for all your valuable feedback. It really helps me a lot!

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Looks good, a few small things and it's ready.

app/queues/dns/dns-flow.server.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
app/queues/dns/create-record-worker.server.ts Outdated Show resolved Hide resolved
@Genne23v Genne23v force-pushed the issue-217 branch 2 times, most recently from f91afbd to 8f2dcb2 Compare February 27, 2023 14:54
@Genne23v Genne23v requested review from humphd and a user February 27, 2023 15:02
humphd
humphd previously approved these changes Feb 27, 2023
@ghost
Copy link

ghost commented Feb 28, 2023

Looks nice!

Thank you for all your valuable feedback. It really helps me a lot!

Cool, I'm happy if you find it useful :)

ghost
ghost previously approved these changes Feb 28, 2023
@humphd
Copy link
Contributor

humphd commented Feb 28, 2023

  • Create a hostedZone using unit test dns.server.test.ts. If you run all test, it will remove hostedZone. Run partial test.
  • Get hostedZoneId from localhost:5053/moto-api and set AWS_ROUTE53_HOSTED_ZONE_ID
  • Set ROOT_DOMAIN to starchart.com

I wrote #283 to help with this logic.

SerpentBytes
SerpentBytes previously approved these changes Feb 28, 2023
app/queues/dns/dns-flow.server.ts Show resolved Hide resolved
app/routes/dev.tsx Outdated Show resolved Hide resolved
@Genne23v Genne23v merged commit 68864ba into DevelopingSpace:main Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: back end Back end part of our web service category: data Anything related to data management and structure category: DNS A service about hosting domains category: queue A service that connects and manages certificate creation/expiration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a workflow to add domain to DNS and DB
5 participants