T2508 Enable user to configure LUA script that alters recursor resolving

[shafer]

This is something I currently use on my VyOS install so I can take advantage of LUA scripts that alter the behaviour of the server.

You can find a few examples here: https://github.com/PowerDNS/pdns/tree/master/pdns/recursordist/examples

There isn't a phabricator for this work. If required to get this accepted I could setup an account and link this PR to a feature request.

[jjakob]

I'm working on T2486 right now which will add a lua file, but not via config. This would then need to be joined with the config-set lua. I can add your changes to my patch set, I'd need to rebase on your work anyway if this PR were merged as there are significant changes needed on my end. You should always create a phabricator task to avoid duplication of work with other people, so they know what they're working on in cases like this, and it's mandatory to have the task ID in the commit messages and PR name to get the patch accepted.

[shafer]

Ok. I can add a task next time. I'm an infrequent contributor and haven't been able to stay caught up with the new processes.

Would you suggest I close this PR? I'm just happy for the feature to wind up in rolling. I don't care how. :)

[dmbaturin]

There's no need to close it. You can force push to the same branch, with correct commit descriptions.

[jjakob]

I see no need to either as I'll incorporate the changes into my current work.

…

On May 24, 2020 7:27:00 AM GMT+02:00, Daniil Baturin ***@***.***> wrote: There's no need to close it. You can force push to the same branch, with correct commit descriptions.

[jjakob]

• I'd name the node 'lua-dns-script' to avoid confusion with 'lua-config-file' (TIL pdns-recursor has 2 different lua config files)
• add the empty variable "lua_script": "", to default_config data
• put a warning in the new node field like there is in dhcp-server *-parameters (e.g. "You must use a file suitable for use with pdns-recursor's lua-dns-script, any errors in the file will result in a service failure. Check the system log for errors after modifying.")

[shafer]

I made a comment in Phabricator already, but I'll make one here as well. I added the suggestions and pushed a new branch.

[jjakob]

You haven't actually pushed any changes?

[shafer]

I shouldn't have said pushed a new branch. I pushed an updated branch. In the "Files changed" section of this PR I'm seeing the empty default, the warning language, and the node name change.

[jjakob]

Well none of the issues I raised in the review are fixed, so the current code won't even work, that's why I suspect it's still waiting to be merged. Double check the comments in my review. To clarify, this was after the first comments that you mentioned (those changes were pushed, but the code still had issues so I added some more comments)

[shafer]

Based on the feedback in this comment I made changes. I don't see feedback other than that.

Here is a screenshot showing what I see. Which includes a new node name of lua-dns-script, a warning similar to making custom dhcpd changes, and the empty default.

I'm at a loss to find other comments in this PR or in Phabricator that suggest code changes.

[jjakob]

That's odd, the code comments are definitely showing here... I'm closing the current review and reopening it in case Github messed up somehow

[shafer]

Please let me know in a comment when you do. As of this comment I'm not seeing anything new yet.

[jjakob]

This variable doesn't exist, it's named lua_dns_script in the main script (the 'dns' dict variable keys are the variables in the jinja2 template)

[shafer]

Fixed this in latest push. I forgot to rename if in template after renaming it other places.

[jjakob]

Still an unneeded variable assignment, you can assign it directly to the end variable like this:
dns['lua_dns_script'] = conf.return_value('lua-dns-script')

[shafer]

I'm aware you can do this, but I was following the pattern already present in the code. My pref would be to leave it as-is and let a different phabricator efffort to clean/optimize handle it.

[jjakob]

Figured it out, it was a PEBKAC on my part, the comments were "pending" until I actually clicked "start review"...

[jjakob]

@shafer one issue was fixed, the other comment wasn't - it'll still work that way, but I don't see the reason for the unnecessary intermediate local scope variable. Assign the result of conf.return_value to the dns dict key directly in one line.

[shafer]

Maybe I didn't do something with the review I was supposed to. I commented that I did it that way because it matched the pattern. I've updated again to use the code that takes fewer lines, but doesn't match the pattern for cache_size or negative_ttl as examples.