Adds DnsWriter that implements DNS UPDATE protocol

[hridder]

A DnsWriter that allows the domain-registry to update a (capable) DNS server appropriately as domains/hosts are created/updated/deleted. It creates RFC 2136 DNS UPDATE messages that are sent via a TCP socket to a single configured DNS server. This server is sometimes called a "hidden master". For each publish call, a single UPDATE message is created containing the records required to "synchronize" the DNS with the current (at the time of processing) state of the registry, for the supplied domain/host. The code assumes, per the RFC, that each update is atomic and that the SOA serial-number is implicitly incremented. Any failure processing a publish call throws an exception, assuming the registry's dns queue task action thingy will retry the request.

The DNS server itself could run anywhere, but we plan to run it in a container or VM in the cloud alongside the registry. It should work with any DNS server implementing RFC 2136 (we are doing our integration with BIND). The DNS server likely requires some "out of band" configuration, such as making it authoritative for the TLD zones.

Major Changes

• DnsUpdateWriter publishes changes to NS, DS, A, AAAA records for domains/hosts as appropriate
• Static configuration separate from RegistryConfig
• Include dnsjava library as new third party dependency to generate DNS protocol messages
• Expose /_dr/task/writeDns in RegistryTestServer

Issues/Questions

• I'd particularly like ideas on how to configure this. My understanding is that this DnsUpdateWriter is an option, so it didn't seem right add the required configuration fields to the RegistryConfig. Therefore it seems some way is needed to modularize the configuration, I simply created a separate one with no real injectability. As a side note, for our purposes we would like to store the configuration external to the application, perhaps in the Datastore, to keep it out of the source code, the built binaries, and otherwise secure since it may contain "secrets". We'd then look it up based on the environment (prod, test, dev, etc.). That or somehow bind it with the app during deployment.
• This is purposely a very simple minded implementation (config issue notwithstanding), which may not be suitable for all uses. We think it's the minimum functionality for our purpose.
• Currently not included in BackendComponent, which must be modified to enable it.
• DnsUpdateConfig, similar to TestRegistryConfig, is hard-coded and has to be modified to use it.
• Each UPDATE message causes a new TCP connection to the DNS server to be opened/closed. This may not scale to crazy big TLDs, but is simple to start with. Testing will reveal if this is a good choice.

[jart]

Please use the @Config annotation.

[hridder]

Done.

[jart]

By the way, I just want to mention that my review has only been partial so far. I'll give it a more in-depth look in the next few business days.

[hridder]

Thanks Justine. I also assume that some of my answers may lead to more questions/suggestions, so have at it. From an review ettiequte perspective, I assume that if I agree with your comment/request, that I don't need to respond, just make the change. In the same vein, how should I/we treat "nits"?

I'm a noob at github, so should I start pushing changes or will that make the review harder?

[jart]

Re: Review etiquette:

For starters, you don't have to take what I say for granted. You are welcome to push back on any comment and we'll have a friendly conversation. However if you agree with me and make a requested change, then it's best that you leave a "Done" comment just to make it clearer that it was made, since sometimes comments can get lost in the review process if there's many of them.

I usually try to start out with a few high level comments, like the thing about sockets on GAE, to avoid going into too much detail if major changes have to be made.

Since this code will be merged into a Google codebase, it'll be held to the same standards as any CL written internally. There will be nits regarding style and best practices. This can be painful at first. However I must say that, at first glance, you've done a remarkably good job avoiding most of them.

When I make nits, I'll make an effort to cite the style guide, with links, whenever possible. Some of our guides are not public. I can probably take screenshots of them if you like. Or point to books like Effective Java. But most importantly, I always try to make it clear when I'm stating personal opinion, rather than explicit company policy.

Sometimes I'll put [optional] in a comment to say that I don't care if you ignore it. But please at the very least consider it.

I take a breadth first approach to review. So usually after the high level comments and nits are out of the way, I start thinking about the behavior of the code in greater depth.

At the end of the review, you'll receive an LGTM. At which point you'll probably need to squash it down to a single commit. Then I'll hit merge.

And that's pretty much all you should need to know about the process. Let me know if you have any more questions :)

[jart]

Regarding configuration:

1. Secrets should be stored using the com.google.domain.registry.keyring package. Like the DNS writer, this has a void implementation that needs to be swapped out by something you guys wrote, that talks to a black box that stores secret values. We recommend something more isolated and secure than datastore.

2. The static configuration system is modular. You can define a module anywhere you want that provides @Config values. We like this configuration system because it's statically linked, type safe, and verified by the compiler. It's very well suited to configuration values that only need to be configured once per deployment.

3. We don't use @Config for everything. If you have configuration values that might be changed more often than once, we recommend implementing them in a manner similar to Registry.java or PremiumList.java. This involves:

   1. Defining a new type of entity, possibly extending CrossTldSingleton
   2. Using a LoadingCache
   3. Writing a tool command for mutating the value

   Naturally, this creates a lot of moving parts that might break when compared to static @Config parameters. So we recommend only using this sort of runtime loading when it's absolutely necessary.

[hridder]

Uh, sorry if my push hammerd your comments. Everything I read said github would handle it.
Regardless, I've made all the changes you noted. Let me know how you'd like to proceed.

[jart]

Imperative tense should not be used for method javadocs (s/Publish/Publishes/). Same applies to constructor. Also class javadoc should be updated to remote "A " prefix. See: https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment

[hridder]

Entire comment pulled up to DnsWriter.

[hridder]

1. CLA bot seems happy.
2. build is clean, test results:

ridder$ bazel test //java{,tests}/com/google/domain/registry/... --test_output=errors --local_resources=8000,4,1.0
INFO: Found 180 targets and 330 test targets...
INFO: Elapsed time: 564.603s, Critical Path: 308.13s
...
Executed 279 out of 330 tests: 330 tests pass.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.

PTAL

[jart]

Can you reply with the text "I signed it!" so we can see what the CLA bot has to say?

[hridder]

I signed it!

[hridder]

Typeo "Send a send a ..."

Let me know if you want me to update the PR...

[jart]

Please do. Also please don't use the imperative mood when writing method javadocs. Prefer "Sends query..." over "Send query..."

[hridder]

Fixed, pushed, PTAL. And in case you hadn't noticed this:

14:29 PDT
Our systems are working through a queue backlog. In the meantime, new pushes may take some time to show on GitHub.com.

[jart]

What is the email address associated with the commit in this PR? GitHub won't show me the email address.

[hridder]

It should (now) be the same as in the CONTRIBUTORS file. hans.ridder [at] gmail.com I did a git commit --amend --reset-author followed by git push origin dnswriter --force to fix it. That seemed to make the CLA complaint go away.

[jart]

Please sign the CLA.

[hridder]

Happy to, but my understanding is that this is supposed to be under the Donuts corporate agreement. Is there some problem with that, or do I need to sign individually also?

[jart]

Do you have an @donuts.co email address?

[hridder]

No, and it was suggested we use our personal email/github so our contributions are tracked on github beyond here. What's odd is that the CLA complaint went away as soon as I pushed the commit with my github email address (the one above).

[jart]

What we do at Google is we use our personal GitHub accounts with our corporate email address. I would recommend the same for Donuts. If Donuts owns the code in the commit, then a donuts email address should be associated with that commit.

I've modified the project settings so that the CLA check is now mandatory before merging. This tool also reports that you need to rebase your change on top of the latest master.

[hridder]

I've updated github and the PR to use the email address that's in the google-group registered with the Donuts CLA. I'm thinking this will fix the issue. Try using my github username hridder in that search box above.

At the moment the status above seems delayed:

[jart]

I don't know what email address you're talking about. What email addresses do you have?

[hridder]

Hi. Yes, I failed to mention the email address, but I'm assuming it doesn't matter since github still thinks it's "Waiting for status to be reported", just as in the image I posted earlier. Previously there was a pretty red "no CLA" in that spot, and the merge status was green. Not sure what's blocking that. However this morning there's a prettry red new_ label of "cla: no" on the PR. Probably most important is that the CLA site doesn't think there's a CLA covering me. Apparently the the document was only signed yesterday, and "normally takes a few days".

The only other thing I can do is to sign the individual agreement, though I'm not sure how long that takes. Oh, the email address is hans.ridder [at] dev9.com, though that probably won't help. And I'd prefer to submit under my gmail account if you don't mind, so once the bots are happy with this configuration I'd like to try switching back before you merge, if that's okay with you.

[jart]

I'm in no rush. It's probably better if it's associated with your
professional address.
On Apr 6, 2016 3:44 PM, "Hans" notifications@github.com wrote:

Hi. Yes, I failed to mention the email address, but I'm assuming it
doesn't matter since github still thinks it's "Waiting for status to be
reported", just as in the image I posted earlier. Previously there was a
pretty red "no CLA" in that spot, and the merge status was green. Not sure
what's blocking that. However this morning there's a prettry red new_ label
of "cla: no" on the PR. Probably most important is that the CLA site
https://cla.developers.google.com/clas doesn't think there's a CLA
covering me. Apparently the the document was only signed yesterday, and "normally
takes a few days https://cla.developers.google.com/about".

The only other thing I can do is to sign the individual agreement, though
I'm not sure how long that takes. Oh, the email address is hans.ridder
[at] dev9.com, though that probably won't help. And I'd prefer to submit
under my gmail account if you don't mind, so once the bots are happy with
this configuration I'd like to try switching back before you merge, if
that's okay with you.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9 (comment)

[hridder]

I'm really hoping to get to 200 messages in this conversation, we're so close! 😃

The CLA site now says I'm covered under Donuts. Is there something you can poke to wake the bot, do we just wait, or should I push a change to trigger a recheck?

Lol, it changed just as I posted this....

[googlebot]

CLAs look good, thanks!

[googlebot]

CLAs look good, thanks!

[jart]

LGTM. This is a fantastic contribution to the codebase. Thank you so much.

[hridder]

I'm glad to be able to contribute, and thank you for reviewing my first contribution. Feel free to reach out if anything comes up about it.

[jart]

Glad I could help. I look forward to your next contribution.