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

PowerDNS Forward-Notify Patch #1701

Closed
wants to merge 23 commits into from
Closed

PowerDNS Forward-Notify Patch #1701

wants to merge 23 commits into from

Conversation

DrRemorse
Copy link

This patch will allow you to redirect PowerDNS notifications to a "passthru" server. It is intended to fix notify/axfr behavior in anycast clusters of PowerDNS auth servers however it may be useful for other situations.

The configuration option "passthru-notify" has been added to the pdns.conf parser. The option accepts multiple IPv4 and IPv6 address values.

This patch obsoletes issue #655.

Ralph Covelli
Hurricane Electric / AS6939
rcovelli@he.net

@klaus3000
Copy link

IMO the feature name is confusing. I would call it "notify-override" (NOTIFY this targets instead of the zones NS set).

@cmouse
Copy link
Contributor

cmouse commented Sep 6, 2014

@DrRemorse Your build failed because pdns.conf-dist differs from output of pdns --config. (Asked in IRC).

@Habbie
Copy link
Member

Habbie commented May 6, 2015

Thank you for this patch!

I'm happy to merge this with some changes:

  1. a rebase (we changed a lot of code on our end, sorry about that)
  2. making sure Travis passes
  3. a rename to forward-notify or something similar
  4. the testIPv6addr function (and the v4 equivalent) almost certainly duplicate code we already have, can you look into cleaning that up? I presume ComboAddress can already do the checking.
  5. clear documentation; right now this feature both forwards incoming NOTIFYs and overrides the target of locally initiated NOTIFYs. Given the override can also be done with a combination of also-notify and only-notify I am somewhat tempted to remove the override feature in full.

Then, we need a review to make sure the static Resolver passthruResolver really is safe, I'm happy to do that.

We're happy to help with any of the above steps if you have trouble with them or can't find the time. Do let us know!

@pieterlexis pieterlexis added this to the auth-4.0.0 milestone Jun 9, 2015
@Habbie Habbie modified the milestones: auth-4.1.0, auth-4.0.0 Dec 15, 2015
@DrRemorse DrRemorse changed the title PowerDNS Passthru Notify Patch PowerDNS Forward-Notify Patch Dec 27, 2016
@klaus3000
Copy link

Which problem are you actually trying to solve? There are plenty of notification options already in PDNS.

@klaus3000
Copy link

Hi DrRemorse! I still do not understand your problem. We also use PDNS in Anycast Clusters but do not have problems. There are plenty of options like notify-only, slave-renotify, ALSO-NOTIFY and what we do, we changed some default SQL queries to a SQL function which has some additional notify logic, so we do lots of notify tricks without any code changes.

Ralph Covelli added 2 commits January 9, 2017 02:33
removed forward-notify from mastercommunicator.cc
must startup communicator if forward-notify is specified
@DrRemorse
Copy link
Author

Hi,

We operate a large anycast cluster of PowerDNS servers as well. Our servers are not configured as masters or slaves. Internally the backend handles synchronization however we can also be configured to slave zones off other user's servers. In this case when the user's server sends us a notification we use this feature to relay that notification to an AXFR controller which then in turn pulls the zone from the user's server. There may be other methods of accomplishing this however we found this way efficient and scalable. We are offering this to the community in the hopes that someone will find it as useful as we did.

Thanks!

@klaus3000
Copy link

Basically our setup may be similar: 1 "control server" PDNS fetching the zone from the customers and receiving NOTIFYs from the customers. Zone are configured with type=SLAVE. The backend is replicated to the anycast nodes (Postgresql+Slony) and the anycast nodes operate with type=NATIVE. To prevent the "control server" to send automatic NOTIFYs to the NS hostnames we use "only-notify=". Customers can also fetch their zones from our control server. Therefore we have slave-renotify=yes and set ALSO-NOTIFY for these customer's zones. So, no code big changes are needed (only one optimizations: #4861)

}
catch(PDNSException &e) {
L<<Logger::Error<<"Unparseable IP in forward-notify. Error: "<<e.reason<<endl;
exit(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't kill the daemon on error

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

It was written like this to be consistent with the behavior of also-notify, only-notify and friends which directly precede this chunk of code. This code is only run once at pdns startup.

Thanks!

@Habbie Habbie self-assigned this Jan 30, 2017
@Habbie
Copy link
Member

Habbie commented Jan 30, 2017

After IRC discussion: needs some squashing, but given the small size of the total change, please feel free to squash it all into one commit.

Copy link
Member

@Habbie Habbie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks to be in good shape. One nit, one question.

@@ -886,7 +888,17 @@ int PacketHandler::processNotify(DNSPacket *p)

// ok, we've done our checks
di.backend = 0;
Communicator.addSlaveCheckRequest(di, p->d_remote);

if(!::arg()["forward-notify"].empty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's cheaper to check if PacketHandler::s_forwardNotify is empty.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Ill make the change and squash tonight.

}
}

if(::arg().mustDo("slave"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional? What is the behaviour change here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional to preserve the original pdns behavior due to my conditional change at the top of the function: if(!::arg().mustDo("slave") && ::arg()["forward-notify"].empty()) ...

Ralph Covelli added 3 commits February 1, 2017 01:32
This patch will allow you to redirect inbound notifications to a proxy server.  It's intended use is in anycast environments where it might be nessessary for a proxy server to preform the AXFR.

The configuration option "forward-notify" has been added to the pdns.conf parser. The option accepts multiple IPv4 and IPv6 address values.
@DrRemorse DrRemorse closed this Feb 1, 2017
@ahupowerdns ahupowerdns modified the milestone: auth-4.1.0 Feb 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants