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.

Sign up for GitHub

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

Jump to bottom

feat: DNS reconciler engine #441

Merged
5 commits merged into from Mar 25, 2023
Merged

feat: DNS reconciler engine #441

5 commits merged into from Mar 25, 2023

Conversation

ghost
Copy link

@ghost ghost commented Mar 24, 2023

No description provided.

@ghost ghost requested a review from humphd March 24, 2023 23:30
@ghost
Copy link
Author

ghost commented Mar 24, 2023

Part of #396

humphd
humphd requested changes Mar 25, 2023
View reviewed changes
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.

Thanks for getting to this so quickly.

This is going to need a bunch of tests in follow-ups. cc @cychu42, @Eakam1007, @Genne23v, @sfrunza13, @SerpentBytes who can probably all lend a hand with reviews and writing tests in the future.

app/lib/dns.server.ts Outdated Show resolved Hide resolved
app/lib/dns.server.ts Outdated Show resolved Hide resolved
app/lib/dns.server.ts Outdated Show resolved Hide resolved
app/reconciler/ReconcilerTypes.ts Show resolved Hide resolved
app/reconciler/createChangeSetFromCompareStructures.ts Outdated Show resolved Hide resolved
app/reconciler/index.ts Outdated Show resolved Hide resolved
app/reconciler/index.ts Outdated Show resolved Hide resolved
app/reconciler/readDbIntoCompareStructure.server.ts Outdated Show resolved Hide resolved
app/reconciler/readRuote53IntoCompareStructure.server.ts Outdated Show resolved Hide resolved
app/reconciler/readRuote53IntoCompareStructure.server.ts Outdated Show resolved Hide resolved
@ghost ghost requested a review from humphd March 25, 2023 02:40
@ghost
Copy link
Author

ghost commented Mar 25, 2023 •
edited by ghost
Loading

Thanks for getting to this so quickly.

This is going to need a bunch of tests in follow-ups. cc @cychu42, @Eakam1007, @Genne23v, @sfrunza13, @SerpentBytes who can probably all lend a hand with reviews and writing tests in the future.

For sure, I know how important this is for the project and the team.

Actually, if this passes reviews, we could completely drop the queue based DNS system, and tie this single call in after any Record table change for now.

I don't think that we have too much traffic on staging, so I'm not too afraid of race conditions until I'm able to finish the scheduler for this. (also, the next reconciliation run would immediately fix any issues if we actually encounter a race condition)

In the meantime, this seems to work, and insert / update / delete recordSets correctly in Route53 ==> the cert generation might just work after we migrate to this (would also immediately kill all drift between our db and Route53 on staging)

@Genne23v Genne23v assigned ghost Mar 25, 2023
humphd
humphd reviewed Mar 25, 2023
View reviewed changes
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 is looking good. I have one question about whether using an instance vs. module functions would improve maintainability, but otherwise, I think we should get others to review this and land it so we an start testing and building the code around it.

app/reconciler/readDbIntoCompareStructure.server.ts Outdated Show resolved Hide resolved
@humphd
Copy link
Contributor

humphd commented Mar 25, 2023

#438 deals with the database side of using multiple values for a given record name+type.

SerpentBytes
SerpentBytes reviewed Mar 25, 2023
View reviewed changes
app/lib/dns.server.ts Outdated Show resolved Hide resolved
@ghost ghost requested review from humphd and SerpentBytes March 25, 2023 17:37
humphd
humphd approved these changes Mar 25, 2023
View reviewed changes
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 sane to me, but without tests I have no idea. Let's try it and build around and on it.

@ghost
Copy link
Author

ghost commented Mar 25, 2023

Thanks everyone for the reviews and suggestions. Your ideas helped a lot!

@ghost ghost merged commit fe5a625 into DevelopingSpace:main Mar 25, 2023
@ghost ghost deleted the issue-396 branch March 25, 2023 21:04
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Genne23v Genne23v Awaiting requested review from Genne23v

@cychu42 cychu42 Awaiting requested review from cychu42

@sfrunza13 sfrunza13 Awaiting requested review from sfrunza13

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@humphd @SerpentBytes @Eakam1007 @denke8