Add upstream strategy strict

[DerRockWolf]

This PR contains a lot of changes (sorry for that 😅)
and closes:

• Use strict order for upstreams #355
• Split ParallelBest into "client groups" and "parallel" #1001

Summary of changes:

• Add upstreams.strategy field
• Add strict upstream resolver (with tests & docs)
• Refactor upstream resolver creation to be "tree-like" -> upstreamTreeResolver (as suggested by @ThinkChaos in Split ParallelBest into "client groups" and "parallel" #1001)
• Refactor: always return Chained(Resolver) and use type assertion in tests if needed

Details regarding the upstremTreeResolver and the "select group by client"

The upstreamTreeResolver contains branches (map[string]Resolver), each branch represents a groups upstream resolver. The upstreamTreeResolver just calls Resolve of the respective branch/group upstream resolver determined by the upstreamGroupByClient func.

Currently a client can "belong" to multiple groups and therefore would use all configured upstream servers (including possible duplicates) where the group name is matching clientName, IP or CIDR.
This behavior is IMO is really confusing and also not really documented or ensured via tests.

I've therefore replaced the "select upstream servers by client" to a "select group by client". It follows a strict order (currently: 1. exact IP, 2. CIDR, 3. clientNames). This would maybe be a breaking change for setups where clients are currently matching multiple groups and would use servers from the matching groups.

What do you think about my proposed change?

Additionally if someone would use e.g. two CIDR groups where an IP matches both, we can't ensure that really the first (as specified in the config file) is used as the config.upstreams.groups field is a map. Same applies for clientNames.

---

Do you want me to also extend the server.go and/or e2e tests? If so please tell me where and to what extend.

TODOs:

• Clarify "select group by client" topic

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.30% 🎉

Comparison is base (39208d8) 93.63% compared to head (3de2e0c) 93.94%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1093      +/-   ##
==========================================
+ Coverage   93.63%   93.94%   +0.30%     
==========================================
  Files          66       68       +2     
  Lines        5345     5513     +168     
==========================================
+ Hits         5005     5179     +174     
+ Misses        265      260       -5     
+ Partials       75       74       -1     

Files Changed	Coverage Δ
config/config.go	76.92% <ø> (-1.29%)	⬇️
resolver/rewriter_resolver.go	96.70% <ø> (ø)
config/upstreams.go	100.00% <100.00%> (ø)
resolver/bootstrap.go	96.81% <100.00%> (+4.15%)	⬆️
resolver/conditional_upstream_resolver.go	100.00% <100.00%> (ø)
resolver/custom_dns_resolver.go	100.00% <100.00%> (ø)
resolver/ede_resolver.go	87.09% <100.00%> (ø)
resolver/filtering_resolver.go	100.00% <100.00%> (ø)
resolver/metrics_resolver.go	100.00% <100.00%> (ø)
resolver/noop_resolver.go	100.00% <100.00%> (ø)
... and 6 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[0xERR0R]

I skimmed the code changes and it looks pretty good so far. 👍
I didn't test it yet, but I'll do it next time. Please check my comments 🤝

[0xERR0R]

I've therefore replaced the "select upstream servers by client" to a "select group by client". It follows a strict order (currently: 1. exact IP, 2. CIDR, 3. clientNames). This would maybe be a breaking change for setups where clients are currently matching multiple groups and would use servers from the matching groups.

This sounds reasonable to me. We should document the order in the documentation to avoid confusions.

[DerRockWolf]

I've therefore replaced the "select upstream servers by client" to a "select group by client". It follows a strict order (currently: 1. exact IP, 2. CIDR, 3. clientNames). This would maybe be a breaking change for setups where clients are currently matching multiple groups and would use servers from the matching groups.

This sounds reasonable to me. We should document the order in the documentation to avoid confusions.

Great. I'll add it to the docs 👍
I hope my proposed order is fine. I don't use multiple groups in my setup.
Maybe switching clientNames & CIDR would be better 🤔
If anyone has use-cases where clients would match multiple groups please let us know!

How should we handle my last concern regarding this:

Additionally if someone would use e.g. two CIDR groups where an IP matches both, we can't ensure that really the first (as specified in the config file) is used as the config.upstreams.groups field is a map. Same applies for clientNames groups.

For now, I think it would be sufficient to add this to the docs. In the future we could validate the config and check for colliding CIDRs & clientNames at startup. WDYT?

[kwitsch]

I think it would also be reasonable to also log it as a warning.

It would speed up support cases where users report unexpected behavior(in my opinion).
If we know that it could happen, let them know. 😊

[DerRockWolf]

I've added the order and further explanation to the configuration docs @0xERR0R
and added a log to warn users if a client matches multiple groups @kwitsch

[0xERR0R]

I tested different configs and it worked fine 👍

Please resolve open points from discussions above.

@kwitsch Do you have any comments regarding this implementation?

[kwitsch]

I tested different configs and it worked fine 👍

Please resolve open points from discussions above.

@kwitsch Do you have any comments regarding this implementation?

I'm currently unable to test it but I've read it and it looks ok so far.

My only cretic would be the change of return types from struct pointers to interfaces as you also mentioned.
It's a bit confusing to read but that could be only me...😅

[ThinkChaos]

Haven't been able to look at this since I don't have any computer time ATM and for a couple more weeks.

It follows a strict order (currently: 1. exact IP, 2. CIDR, 3. clientNames)

Instinctively I'd expect the client names to override CIDR since it seems more specific, so could be used to override a CIDR.
I don't use theses features though, so you probably know best, just thought I'd give a small bit of input.

[kwitsch]

Haven't been able to look at this since I don't have any computer time ATM and for a couple more weeks.

Same as me. Without the GitHub App on my phone I couldn't participate at all right now...

It follows a strict order (currently: 1. exact IP, 2. CIDR, 3. clientNames)

Instinctively I'd expect the client names to override CIDR since it seems more specific, so could be used to override a CIDR.
I don't use theses features though, so you probably know best, just thought I'd give a small bit of input.

Would have said the same but I also don't use it. 🤷‍♂️

[0xERR0R]

Makes sense. Client names should have higher priority. 👍

[kwitsch]

Open changes as discussed:

• Change return types to struct pointers
• Hostnames should have priority over CIDR

[DerRockWolf]

I've refactored every resolver (except upstream_tree & rewriter as they can also return a different type) to return a struct pointer
and changed the order of the "select group by client" logic (also updated the docs). 🙂

[kwitsch]

I'm still unable to test it but I've read the code and cross checked against the previous behavior.

The changes are reasonable, documented and covered with unit tests.

👍

[0xERR0R]

@DerRockWolf: Great job, thank you!