Failback immediately if reset-after is set to 0 and new flag to fail on empty responses

[folbricht]

Based on #444

• If reset-after is set to 0 in the config, the failback and random groups will reset immediately
• A new empty-error flag to specify if empty (or all CNAME) responses should be considered errors. Supported by failback, random, and fail-rotate

TODO: Update the docs

[Anuskuss]

I appreciate that you're copying my commits but do you really want "Anuskuss" ("anal kiss") to show up in your repo history? Just take the code please, it was flawed anyway.

• Can you not do this please?
  

  routedns/failback.go

  Line 176 in 2279e35

  for _, rr := range a.Answer {

  
  If you do e.g. dig @8.8.8.8 ANY google.com you get 22 (!) answers. Do you really wanna check all of them? That seems like a waste of CPU cycles. I've been running this for a few days now and I haven't found any odd cases yet. If I do I'll let you know but first+last check is fine. Also the current logic makes it not clear that it also cares about no answers at all (it's only implicit).
  Edit: Okay, I misread the code. It breaks once it finds a non-CNAME record, so it doesn't "check all 22 answers". I still liked the old code better though, and it's faster for the case where it's 3+ CNAMEs (which I got while testing).
• I found a new case which hasn't been handled yet:

if r.opt.EmptyError {
   switch aLen := len(a.Answer); aLen {
   	case 0: edns0 := a.IsEdns0(); if len(edns0.Option) == 0 || edns0.Option[0].Option() != 15 { return false }
   }
}

You could also do !(>&&==) though if you prefer that (it's more code).

[folbricht]

Can you explain what that extra case should do? The way it's written, it'll already return false when the length of the array is 0, that code isn't going to change that. Trying to understand what the intention is there.

As for using the loop to find any non-CNAME records, the overhead is completely negligible, this won't even show in a CPU profile. I think it's worth for the extra "correctness", no relying on a specific order. This isn't a scripting language so it really doesn't matter in this context.

[Anuskuss]

Trying to understand what the intention is there.

Well in the earlier code at the end it would return true when it hasn't been catched by any condition. Zero answers (case 0) would usually return false (!isSuccessResponse) unless EDE15 was set. This handles the case where I deliberately return 0 answers (from a blocklist) but I don't want that to failback.

the overhead is completely negligible

Well, cycles are cycles. But if you think it doesn't matter, it probably doesn't matter.

[folbricht]

The logic is becoming too complex and convoluted, I'd rather not add yet another condition based on EDE15. It's quite custom and getting very hard to understand.

This may be a another case where having a component whose behavior can be customized by config would be better. I should look into making an element that allows users to provide a Lua snippet to control behavior.

[Anuskuss]

Well but EDE15 is a deliberate "empty answer" so I personally can't make use failback like this because my filtered resolver (Cloudflare + EDE15 blocklist) will make it failback once the blocklist is hit. I mean the end results is technically the same (i.e. a request was blocked) but in this case I'd lose my EDE text.

[folbricht]

Can you move the blocklist in front of failback so the response is only blocked once it's gone through failback?

[Anuskuss]

There are also DNS services with internal blocklists, e.g. Cloudlflare for Families, which can return EDE 15 - 17. If the first request to 1.1.1.3 gets blocked, it'll fallback to 1.0.0.3, getting the same empty response and thus falling back again. So it could potentially turn a single query into four.
So I think that this not only deserved to be handled in failback and friends, but should also be extended to handle 16 and 17.

[folbricht]

I added that check (for all 3 codes), but no time to fully test it. Would you be able to try it out?

[Anuskuss]

That did the trick but reset-after = 0 does not work because of this code:

routedns/failback.go

Lines 119 to 121 in 3255afe

	if r.opt.ResetAfter == 0 {
	return
	}

[folbricht]

Hmm, what's wrong with it? Seems correct, it doesn't set a timer when set to 0, which means it'll basically fail-over only for individual requests and start with the same resolver every time which is identical to failing back immediately.

[Anuskuss]

Well it doesn't do what it's supposed to do. It works with anything other than 0 (e.g. -1) or if I comment out the code.

[folbricht]

Oh, right. Found the issue and pushed a fix. Try again (with reset-after = 0)

[Anuskuss]

👍