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

Roundrobin DNS for a shared hostname #34

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

timothyclarke
Copy link

@timothyclarke timothyclarke commented Mar 8, 2021

Creating DNS for multihost entries aka roundrobin DNS.
Updated to TF 12

This addresses #33

For the repo owner
If you want people such as myself raising PR's please consider adding CODEOWNERS and Contributing.md files.
The CODEOWNERS will auto add reviewers and Contributing.md would detail any other process / code quality etc that submitters should follow

@timothyclarke timothyclarke marked this pull request as ready for review March 9, 2021 13:48
@timothyclarke
Copy link
Author

@jimsheldon @hikerspath @spier Is there any chance this can get a review and merge please ?

@jimsheldon
Copy link
Contributor

@timothyclarke sorry for the delay on this, we will look into this asap

@spier
Copy link
Contributor

spier commented Jun 16, 2021

Btw congrats @timothyclarke on your first PR in the meltwater GitHub org :) 💯

@timothyclarke
Copy link
Author

Thank you

@jimsheldon
Copy link
Contributor

Hello @timothyclarke sorry again for the delay on this, I am working on this now.

Can you help me understand the use case for this? The linked issue says:

Since all nodes in ASG has the same name, this could also be used as a less expensive alternative to load balancers.

We have always used load balancers if we wanted this kind of behavior, I want to understand why this approach would be better.

Thanks!

@timothyclarke
Copy link
Author

@jimsheldon
The exact text might need a little tweaking. In that context loadbalacer meant ALB, NLB or a Classic Loadbalancer. There are occasions where other loadbalancers are in use.
Additionally there are times where a simple roundrobin DNS is sufficient (that's what most global loadbalancers are)

The reason that i looked for this code (and submitted this patch) was because we had an old couple of servers which could run in an ASG. These had previously been outside AWS and was using haproxy for both loadbalancing and url re-writing. The rest of the stack was migrated into K8s but these instances needed to stay around for a while before the code was strangled. The SSL offload, url rewriting and loadbalancing was moved into the k8s cluster which left just a pool of servers that needed to be in an ASG.

A point to note I found with the original code is that it had a race condition when cycling an ASG when you were switching to a new golden image. I noticed that at on a number of occasions the new instance would start, attempt to register DNS, fail (or over write the existing entry), then it would pass health check, the older instance would be removed from the ASG and it's removal would trigger the removal of the DNS entry. The amended code allows for this overlap

@jimsheldon
Copy link
Contributor

Thanks for the background @timothyclarke

We do have a simple test for the current functionality written in terratest, would you be comfortable writing an additional test for this new functionality?

@jimsheldon
Copy link
Contributor

We are interested in integrating this change, but it looks like it has conflicts now.

Hopefully you are enjoying summer 😄, please respond when you can and we will collaborate on this.

@foymikek
Copy link

foymikek commented Apr 7, 2023

Hi @timothyclarke, we have revisited your request and can empathize with the value of the possible feature. While we think there is value in its addition, we have chosen not to include it as we see it as outside of the scope of the module. If you still are having issues and are looking for a work around we recommend using a load balancer to ensure round robin functionality.

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.

4 participants