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: Handle DNS multiple A Records #57

Closed
wants to merge 6 commits into from

Conversation

chakflying
Copy link
Collaborator

@chakflying chakflying commented Jul 16, 2021

Greetings fellow HKer!

Using multiple A Records in a DNS result is a well established method of providing redundancy. The problem is that Node.js's networking is one of the few that does not handle this gracefully (discussed in here almost 5 years ago). If one of the servers Timeout and node happen to request it first, it will be shown as DOWN, but browsers can visit the site no problem since they will try all the servers simultaneously. In my case it's unfortunately a deal-breaker as I don't have full control over the DNS, and one of the IPs returned is always down.

Here I have worked out a minimal viable fix for the problem by using async to try all the servers in parallel, and return if one of them has a successful response. Ideally I think the UI should also support displaying multiple servers for a hostname, but I would like to know your thoughts on that first.

@louislam
Copy link
Owner

Interesting, I never know failover can be done in this way. This is a good question, and also, it is a big question in my opinion. We need some discussions before merging.

Is dns failover really a standard way for all browsers on all platforms? Because I saw someone on stack overflow said the browsers are not doing automatic DNS failover.

https://stackoverflow.com/questions/5319649/using-dns-for-failover-using-multiple-a-records

Secondly, I am thinking some cases. If a user is monitoring a REST API rather than a website and with your situation that one of the server timeout. I think it is appropriate to say it is an unstable API, because this API is not going working properly if the request client is using Node.js. Uptime Kuma should not shows "UP" in this case as I think. And I checked Java also only resolves the first IP like Node.js.

I am also thinking Puppeteer (Chrome headless) maybe a better solution for your case. Users can choose axios or Puppeteer when creating the monitor. The drawback is Uptime Kuma will become a "resources eater" once I add Puppeteer dependency and even longer docker build time, which I don't want to.

Btw, glad to see a person from my hometown and same university department, contribute to Uptime Kuma, lol.

@louislam louislam added the question Further information is requested label Jul 16, 2021
@chakflying
Copy link
Collaborator Author

Regarding browser support, according to this more up-to-date answer the mechanism is specified in the RFC "Happy Eyeballs version 2", and is implemented in all major browsers. I tested this works on my Chrome and Firefox, which should be the vast majority of users. But I totally see your point as it seems the adoption for non-browser clients have been slow. Golang still has an open issue on this, Ruby only implemented this last year. So I guess it doesn't make sense to just make this transparent and show "UP" when only some requests go through.

For puppeteer, I think it would be useful for people who want a more advanced version of the "keywords" function, maybe some DOM parsing or SPA support? But in my case I don't see the need for bringing in a whole browser just for this.

Reading issue #56 made me think a new "disrupted" status would also make sense for this situation. But the monitoring code would probably get more complicated. I could take a shot at implementing it if you're interested in that direction.

server/util-server.js Outdated Show resolved Hide resolved
server/util-server.js Outdated Show resolved Hide resolved
@chakflying
Copy link
Collaborator Author

Closing this for now since I committed to master by mistake.

@chakflying chakflying closed this Jul 22, 2021
@CommanderStorm CommanderStorm added the area:monitor Everything related to monitors label Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:monitor Everything related to monitors question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants