feat: support CNAME records in customDNS mappings

[BenMcH]

This PR is related to #465. Currently, I have only implemented CNAME records, but it would be simple to add additional record types mentioned in #996.

Please let me know if you'd prefer a different approach, but this PR being merged would allow me to replace pihole with Blocky in my home network. I'm happy to add more record types if they make sense. My understanding of DNS is that most other record types typically have more complex use-cases that may not make sense to implement, as they're usually reserved for serving data such as mail configuration or DNS ownership, neither of which I would expect Blocky to be utilized for, given its local-network scope and risk associated with hosting a publicly accessible DNS server.

Further DNS response types can be added with a simple if statement in both the custom_dns.go and custom_dns_resolver.go.

[ThinkChaos]

Thanks for picking this up!

I've got a couple comments about details, but also wanted to bring up maybe we should use dns.ZoneParser which supports the standard zonefile format instead of making something custom? UnmarshalYAML could error if any RR is not an A/AAAA/CNAME.
That also would have the benefit of allowing to split the mapping into another file via $INCLUDE.
That shouldn't be a blocker for this PR though.

[kwitsch]

Agreed that the zone parser looks promising but is out of scope and could be implemented later on.

[BenMcH]

Thanks for the review @kwitsch and @ThinkChaos! I'll get these changes implemented tonight 😄

[ThinkChaos]

A couple new minor things and some follow-up from the previous threads.
Progress is looking great :)

[BenMcH]

I think that I've implemented most of the PR feedback. I'm not a regular golang user and have been having a heck of a time implementing the CustomDNSEntries unmarshalling. I didn't want to force the rest of the changes to come after that, so I skipped it for now. I'll keep hacking away at it on and off tonight, but if you feel this is ready to ship, I can do a follow-up PR once I figure it out.

[ThinkChaos]

If you want to post something that's blocking you feel free and I can take a look :)

I resolved most threads, there's just that and one error in processIP left, and then I think you'll get some trouble from the linter.

[BenMcH]

If you want to post something that's blocking you feel free and I can take a look :)

I'm mostly just not sure how to get the nested unmarshalling working. I took the sample posted earlier and tried to run with it. Conceptually, the code makes sense to me, however, when I run it, the UnmarshallYAML function gets called and I'm getting empty strings as the input. I'm sure that I've simply mapped something incorrectly.

Here's a gist of the error: https://gist.github.com/BenMcH/32137efc5f5d5f0ce9f33bc94ccda796

I resolved most threads, there's just that and one error in processIP left, and then I think you'll get some trouble from the linter.

Sorry for my ignorance, but which error in processIP am I missing? I thought that I had updated all of the calls that could return an error to bubble the error up and out of the function.

I'll take a look at the linter soon! 🚀

[ThinkChaos]

I'm mostly just not sure how to get the nested unmarshalling working.

The code you posted works for me!
I see one potential issue: the fmt.Println("Here", input) runs before unmarshal(&input) so it'll see input as an empty string. So maybe it's your debugging that's misleading you?
And another minor thing, but that shouldn't break anything: result := make(CustomDNSEntries, len(input)) should be result := make(CustomDNSEntries, len(parts)).

Sorry for my ignorance, but which error in processIP am I missing? I thought that I had updated all of the calls that could return an error to bubble the error up and out of the function.

The one from this thread: #1352 (comment)
rr, _ := ... should become rr, err := ..., and then do the usual err != nil dance.

[BenMcH]

Thank you for all of your help on this, @ThinkChaos! Turns out I was overthinking it and failed to assign *c to result at the end, which made my DNS queries fail 🤦‍♂️

I'll go ahead and implement the rest of the feedback tonight.

I think that the processIPs change is implemented. The current diff has the following:

rr, err := util.CreateAnswerFromQuestion(question, ip, r.cfg.CustomTTL.SecondsU32())
if err != nil {
	return nil, err
}

[BenMcH]

Alright, make lint is now happy locally. I think the tests should be fine as well, but I could be wrong. When I build from main or my branch, reverse DNS is not functional, which could probably be attributed to something in my home network or my blocky config (I don't typically use reverse lookups, so this is the first I've tried them at home).

The reverse DNS and old log file deletion tests are failing on both branches on my machine, so I don't think that I introduced it. If I did introduce them, let me know, and I can continue looking into it.

[BenMcH]

Seems I just had to walk away and come back with fresh eyes. I was able to fix the reverse DNS tests and now both make lint and make test work locally!

[BenMcH]

Alright, I put the create answer function on the custom resolver and made a setter func, like I saw in the NextResolver struct and forced an error for the create answer path. My local coverage.html file now shows that the file is 100% covered 😁

[kwitsch]

Looks good to me.

One small comment how to possibly prevent CNAME loops.

Nice work & communication. 👍

[ThinkChaos]

Can you delete the blocky binary and add it to .gitignore?

[BenMcH]

@ThinkChaos thanks for the thorough review! I think that I've implemented all of your suggestions. We now conditionally resolve A or AAAA records on CNAMEs based on the question type and we prevent blocky from starting if there is more than 1 mapping for a domain and any of them are a CNAME.

[BenMcH]

Sorry, I should have run lint locally before pushing. Now it should pass 🤞 I verified both tests and lint

[ThinkChaos]

Ok last round for me I think :)

[ThinkChaos]

IDK why we need to re-approve the CI on every push, I don't remember it being like that before. Maybe because it's your first MR to this repo?

[BenMcH]

IDK why we need to re-approve the CI on every push, I don't remember it being like that before. Maybe because it's your first MR to this repo?

That would make sense. Some github actions are things that you wouldn't want to run unless the code has been reviewed/trusted unless the user is a contributor/trusted. I've been making do with the makefile targets locally though 😄

[kwitsch]

IDK why we need to re-approve the CI on every push, I don't remember it being like that before. Maybe because it's your first MR to this repo?

That would make sense. Some github actions are things that you wouldn't want to run unless the code has been reviewed/trusted unless the user is a contributor/trusted. I've been making do with the makefile targets locally though 😄

As far as I observed the behavior:
Until your first MR is merged the action runs need reaproval on any change.
If the first MR is through you are somwhat trusted according to GitHub. 😉

[0xERR0R]

As far as I observed the behavior: Until your first MR is merged the action runs need reaproval on any change. If the first MR is through you are somwhat trusted according to GitHub. 😉

First time contributors need always approval (this is some kind of protection from GitHub to avoid execution of project workflows on each PR) 🤷

[BenMcH]

I think that the e2e test failure may have been a false positive. The errors reported were for an http port timeout and an error communicating with mariadb. I ran e2e tests on my local machine this morning and all tests passed.