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
Lua2 backend #6157
Lua2 backend #6157
Conversation
7e62cc9
to
055c1af
Compare
55fe1b2
to
c148963
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a full review, just some comments. Can do a better review later. To be clear, I am fine with merging this as is if it does not affect anything outside of the backend.
docs/backends/lua2.rst
Outdated
The backend uses AuthLua4 base class, and you can use same functions and types as in any other Lua script. | ||
|
||
.. warning:: | ||
Some of the function calls and configuration settings have been changed, please review this document carefully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed compared to what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compared to lua backend. Need to clarify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
docs/backends/lua2.rst
Outdated
All places which use DNS names now use DNSName class which cannot be compared directly to a string. | ||
To compare them against a string use either ``tostring(dnsname)`` or ``newDN(string)``. | ||
|
||
API description (v1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe call this v2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, not sure. I'm happy with any way you decide. =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with v2 to avoid any confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed. Also added check that if version 1 is used, it instructs to use luabackend instead.
docs/backends/lua2.rst
Outdated
Perform lookup of given resource record name and type. | ||
|
||
INPUT: | ||
- string qtype - Type of queried resource record |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not an integer or a qtype type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uses QType now.
docs/backends/lua2.rst
Outdated
- DNSName name - resource record name (can also be string) | ||
- string type - type of resource record | ||
- string content - resource record content | ||
- int ttl - time to live for this resource record (default: configured value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would one trigger the defaults?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clarified documentation
- int ttl - time to live for this resource record (default: configured value) | ||
- int domain_id - ID of associated domain (default: -1) | ||
- bool auth - Whether data is authoritative or not (default: true) | ||
- int last_modified - UNIX timestamp of last modification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for, if not for autoserial?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i included all fields on the structure.
- int domain_id - ID of associated domain (default: -1) | ||
- bool auth - Whether data is authoritative or not (default: true) | ||
- int last_modified - UNIX timestamp of last modification | ||
- int scope_mask - How many bytes of source IP netmask was used for this result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What use is this if lookup
does not get passed the source IP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I could pass the DNSPacket here... or maybe a special table with values of interest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Table makes more sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
special table added.
docs/backends/lua2.rst
Outdated
- DNSName domain - Domain to get info for | ||
|
||
OUTPUT: | ||
Return false if not supported or found, otherwise expects a table of: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment also applies to other functions: should I return a table with these things in this order, or do I return a table with keys account
, kind
, etc.?
@@ -80,6 +80,7 @@ void loadMainConfig(const std::string& configdir) | |||
string configname=::arg()["config-dir"]+"/"+s_programname+".conf"; | |||
cleanSlashes(configname); | |||
|
|||
::arg().set("resolver","Use this resolver for ALIAS and the internal stub resolver")="no"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why add this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one is required by lua-auth4.cc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is ok..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
docs/backends/lua2.rst
Outdated
- DNSName qname - DNS name to calculate | ||
|
||
OUTPUT: | ||
Table of three DNSNames, in order of Unhashed, before and after. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding some previous comments, I see from the testing script that some functions indeed expect keyed tables. This one does not - suggest saying 'Array of three DNSNames' for clarity.
These are needed to implement lua2 backend
Needed by lua2 backend
This is API version 1 of lua2 backend. It provides improved interface for Lua script to act as backends. Configuration - `lua2-filename` - path to script - `lua2-query-logging` - log lua queries and results - `lua2-api' - API version (default 2)
Short description
This is a rewrite of the lua backend. It uses AuthLua4 as basis and more strongly typed access using LuaContext.
Checklist
I have: