Add IPv4/v6 dual-stack to the configuration

[jorhett]

Add to plugin

• address_type for address types supported
• checkip_name_v6 for IPv6 lookup

Add to provider configuration

• addrtype for type of address to submit
• checkip_cmd_v6 for IPv6 command to lookup IP

NOTE: This adds the necessary types to the configuration. It does not (yet) change the behavior of the stack. I wanted to stop here and get feedback from @troglobit on whether or not I'm going in the right direction, or if my rusty C skills are making him puke ;-)

When done will address #52

[troglobit]

It's almost like riding a bike :)

In general I think the PR looks good. I have a few comments though:

• I'd prefer a different namespace for the new additions: e.g. checkip6_foo for variables and checkip6-foo for configuration parameters
• The constructs with strstr() looks a bit fragile, but I have no other suggestion so let's just go with that for now
• Currently it doesn't build, but I guess you're on top of that. If not, see the Travis-CI build (red 'x') above

Also, it'd be great to get some feedback from @acolomb who I know have been working on this as well.

Minor note, when finalizing your commits for this pull request, add one (relevant) that has "Fix #52: some-descriptive-text". Makes it easier for me to track and automatically close GitHub issues when I merge the pull request :)

[jorhett]

So the reason I didn't use a different namespace is because it would appear that only two parameters need to be changed. All the other variables in that namespace can be shared AFAICT.

I can still use the different namespace, but it wouldn't be obvious to the eye that it is shared.

[troglobit]

Fair enough, I guess I can add an alias later on if it bothers me :)

[troglobit]

Guys, how should we proceed with this one? @acolomb @jorhett

[acolomb]

I couldn't ponder about it further, but so far I think it would not really disturb my plans. Keeping one or two legacy option names for backwards compatibility wouldn't make much of a difference. But it's not thought through to the end.

[troglobit]

OK, thanks for the feedback @acolomb! I'd really like to get something in there since this has been an oft asked question, so if we can reach 70% of use-cases I'd be very happy.

[troglobit]

Maybe we should finalize work on this. I'm willing to help out to push this through the door, what about you @jorhett ?

[nihilus]

I am looking forward to this, as the inadyn-mt has had support for some time but no SSL, and it is kinda broken in other ways.

I ended up trying to patch inadyn-mt for using libcurl, to no good.

However I didn't realize that this fork uses the native tcp.c stuff.

[nihilus]

@acolomb the only thing I see missing is a fixup for cache.c which itself has an IPv4-only nslookup() function.

[jorhett]

Maybe we should finalize work on this. I'm willing to help out to push this through the door, what about you @jorhett ?

I'm totally onboard and as of this week have time to work on it again ;-) Every single one of my nodes is either dual stack or v6 only so I've got lots of testing options.

[troglobit]

@jorhett Awesome, great to see you again! :-)

[nihilus]

I will check the fix out tomorrow at work, and try to fix cache.c (should be pretty much a cut 'n paste from the sntpd code).

[acolomb]

Sorry I've not been following up on this project. Family life has been keeping me way too busy to get some coding done.

No objections from my side for going ahead with this approach. My "big plans" have always been about making IPv6 a first class citizen and give more explicit control, especially when IPv4 is not even involved. However, the current "tacked on" state of IPv6 support can definitely be improved as well.

[troglobit]

Amazing feedback suddenly in this tread! :heart:

Remember to sync with each other, use this PR if you like.

[nihilus]

One issue is "ddns.c:125:37: error: 'ddns_info_t' has no member named 'checkip_url_v6'"

[jorhett]

That's a very old patch. I'll push a rebase plus namespace changes that @troglobit recommended this weekend. Let's discuss from that point.

[nihilus]

@jorhett, neat

[nihilus]

I must say I am not totally confident how 'addrtype = ' works, should it be 'addrtype = 4,6' for dual updates?

[nihilus]

It should also be noted that FreeDNS suppports IPv6 perfectly fine.

[nihilus]

Hate to be the party crasher but I found some obvious flaws in it and the current code. inadyn should in a dual-stacked environment update both A and AAAA records (if it doesn't have an IP it should use 0.0.0.0 and ::, to make sure end-to-end DNS works), it should also create a socket of respective type when connecting to the server to resolve it's IP addresses.

[troglobit]

With "current code", do you mean current master or the code in this pull request?

[nihilus]

What I am using is a "mishmash" based on current dev-branch, combined with this PR and some fixes for the company I work at (nothing special really, just disabling SSL-strict by default and disable the caching and reading IPs from flat files, some extra syslogging which doesn't really makes sense and rudimentary i18n-support, and using the proxy-interface for SO_BINDTODEVICE).

[troglobit]

OK. Looking forward to reviewing your take on things.

[nihilus]

I can give an update, I did duplicate much code and elements in the structs. Implemented request_v6, response_v6, and setup_v6 (I know that passing a boolean or piggybacking more on the adresstype introduced in this PR would be more appropriate). I also created specific IPv4 and IPv6 sockets by passing the family to the http_init(). The biggest pitfall was that if getaddrinfo() in tcp.c fails we must set it to AF_UNSPEC and try again (FreeDNS is ipv4-only but supports quad-A records). Dynu worked out of the box for me. I will test some more providers tomorrow.

Also I made sure not to assume the path for IPv6 would be the very same; it is a matter of provider implementation and might change in the future.

[nihilus]

And I must say this code is like rewrite of the original code, the old code was such a PITA I quickly gave up making it work with IPv6.

[troglobit]

@nihilus That's fine, as long as the patches are readable and doesn't mess up the IPv4 code paths (more than necessary). Really hoping to get this into In-a-dyn soon, since it's the main blocker for the next release.

[nihilus]

Well, I did find it working but there are some buffer overruns and the code is pretty intrusive.

[troglobit]

@nihilus Sorry to hear that. Do you have any plans on trying to fix it and submit here, or do you recommend we proceed with the current code and "fork" our two efforts? Like I said, I consider the IPv6 issues the main blocker for the next release, which is quite delayed by now. So I'd like to get some sort of commitment, so I can make a decision going forward.

[nihilus]

Yes, but I don't think I'll have something ready for a pull request before january.

[troglobit]

@nihilus OK, I'll hold off the release until then. Looking forward to it!