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

dnsdist: Make custom LuaAction and LuaResponsAction second return value optional #6363

Merged
merged 7 commits into from Mar 20, 2018

Conversation

Projects
None yet
3 participants
@chbruyand
Member

chbruyand commented Mar 19, 2018

Short description

Make custom LuaAction and LuaResponsAction second return value optional.
This allows shorter return syntax for custom lua actions : return DNSAction.None instead of return DNSAction.None, "".

Fix #6346

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • [ ] added or modified unit test(s)

chbruyand added some commits Mar 16, 2018

Merge branch 'master' into issue-6346
* master:
  use mirror for dnsperf
  rather than crash, sheepishly report no file/linenum
  dnsdist: Update deprecated syntax used in dist configuration file
  Update secpoll
  build lua2 backend packages
  calidns: Add the --ecs parameter to add random ECS values to queries

@chbruyand chbruyand added this to the dnsdist-1.3.0 milestone Mar 19, 2018

@rgacogne

Looks very good, just a few nits.

@@ -24,7 +24,7 @@
class LuaAction : public DNSAction
{
public:
typedef std::function<std::tuple<int, string>(DNSQuestion* dq)> func_t;
typedef std::function<std::tuple<int, boost::optional<string> >(DNSQuestion* dr)> func_t;

This comment has been minimized.

@rgacogne

rgacogne Mar 19, 2018

Member

Just a nit but dq made more sense than dr here :)

These constants represent an Action that can be returned from the functions invoked by :func:`addLuaResponseAction`.
* ``DNSResponseAction.Allow``: let the query pass, skipping other rules

This comment has been minimized.

@rgacogne

rgacogne Mar 19, 2018

Member

s/query/response/

* ``DNSResponseAction.Allow``: let the query pass, skipping other rules
* ``DNSResponseAction.Delay``: delay the response for the specified milliseconds (UDP-only), continue to the next rule
* ``DNSResponseAction.Drop``: drop the query

This comment has been minimized.

@rgacogne

rgacogne Mar 19, 2018

Member

s/query/response/

The ``function`` should return a :ref:`DNSAction`. If the Lua code fails, ServFail is returned.
The ``function`` should return both a :ref:`DNSAction` and its argument `rule`. The `rule` is used as an argument
of the following :ref:`DNSAction`: `DNSAction.Spoof`, `DNSAction.Pool` and `DNSAction.Delay`. As of version `1.3.0`, you can
ommit the argument. For earlier releases, simply return an empty string. If the Lua code fails, ServFail is returned.

This comment has been minimized.

@rgacogne

rgacogne Mar 19, 2018

Member

s/ommit/omit/

::
function luarule(dq)
if(dq.qtype==35) -- NAPTR

This comment has been minimized.

@rgacogne

rgacogne Mar 19, 2018

Member

Wouldn't dnsdist.NAPTR work here?

.. function:: addLuaResponseAction(DNSrule, function [, options])
.. versionchanged:: 1.3.0
Added the optional parameter ``options``.
Invoke a Lua function that accepts a :class:`DNSResponse`.
This function works similar to using :func:`LuaResponseAction`.
The ``function`` should return a :ref:`DNSResponseAction`. If the Lua code fails, ServFail is returned.
The ``function`` should return both a :ref:`DNSResponseAction` and its argument `rule`. The `rule` is used as an argument
of the `DNSResponseAction.Delay`. As of version `1.3.0`, you can ommit the argument (see :func:`addLuaAction`). For earlier

This comment has been minimized.

@rgacogne

rgacogne Mar 19, 2018

Member

s/ommit/omit/

"""
Advanced: Short syntax for LuaAction return values
"""
name = 'refused.advanced.tests.powerdns.com.'

This comment has been minimized.

@rgacogne

rgacogne Mar 19, 2018

Member

Would you mind using a different qname than the previous test? It often makes debugging easier.

@rgacogne

LGTM!

@rgacogne rgacogne merged commit 02d3e20 into PowerDNS:master Mar 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chbruyand chbruyand deleted the chbruyand:issue-6346 branch Mar 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment