TuxInvader / nginx-dns

Sample Configuration for DNS over HTTPS (DoH/DoT gateway) and GSLB with NGINX
BSD 2-Clause "Simplified" License
194 stars 47 forks source link

js_include directive deprecated in njs 0.7.1 #14

Closed mariamatthews closed 2 years ago

mariamatthews commented 2 years ago

Since njs 0.7.1 update, code part of:

  # Include the NJS module
  js_include /etc/nginx/njs.d/nginx_stream.js;

will no longer work. Could the code be updated to use js_import instead?


Source: https://nginx.org/en/docs/njs/changes.html#njs0.7.1 Source 2: https://nginx.org/en/docs/http/ngx_http_js_module.html#js_import

cookiemonsteruk commented 2 years ago

I'm glad I saw this. I am on version 0.7.1 of njs on ubuntu. Replacing js_import for js_include solved my nginx config test failure. I now have one with export statement is required in nginx_stream.js:55 which currently reads js_preread dns_preread_dns_request; I'm currently trying to use nginx-dns-routing.conf for testing and trying to get over these issues. Any ideas what I need to do?

cookiemonsteruk commented 2 years ago

From this explanation regarding export statement it appears to my untrained eyes that export statement is needed in njs.d/nginx_stream.js @mariamatthews @TuxInvader As a test I did add the following line below import dns from './dns/dns.js'; export default {dns_get_qname, dns_get_response}; just to test if nginx config stopped complaining and it did. However I don't know if these are the only ones or actually the ones functions that need exporting back. Do I need do do this/something else to get it working? p.s. I'm not a developer, only a user trying to get an nginx gateway working for DoT.

TuxInvader commented 2 years ago

Hi @mariamatthews @cookiemonsteruk

Thanks for raising this.... I think this deprecation has caught a few NJS projects out. 18 months just isn't long enough for some people ;-)

I'll update the code and examples to start using import rather than include. The include file was just a wrapper to import functions from libraries anyway. The fix is simple enough......

Replace:

js_include /etc/nginx/njs.d/nginx_stream.js;

with:

js_import /etc/nginx/njs.d/dns/dns.js;

Then rename the functions from dns_ to dns. Eg: use dns.filter_doh_request instead of dns_filter_doh_request

Cheers

cookiemonsteruk commented 2 years ago

Great.. So the export statement is not necessary?

TuxInvader commented 2 years ago

@cookiemonsteruk It looks like you've tried to convert the nginx_stream.js to a module (which should work), but as NGINX has import natively it's an unnecessary wrapper.

If you look at one of the functions you exported:

function dns_get_qname(s) {
  return dns.get_qname(s);
}

It just passes the call through to the DNS module, so it's simpler to just import the DNS module instead.

cookiemonsteruk commented 2 years ago

Right, got it ! . To clarify though, this bit @TuxInvader : Then rename the functions from dns_ to dns. Eg: use dns.filter_doh_request instead of dns_filter_doh_request Is that in /etc/nginx/njs.d/nginx_stream.js ? Pretty sure is the only place I can see them but I don't want to make assumptions.

TuxInvader commented 2 years ago

Hi @cookiemonsteruk

Make the changes inside your nginx.conf.

Instead of importing njs.d/nginx_stream.js you want to import njs.d/dns/dns.js and then where you use the dns_get_qname change it to dns.get_qname and repeat for any other functions you use. So you don't need nginx_stream.js in your life anymore :-)

cookiemonsteruk commented 2 years ago

Thanks again. I've got an error in nginx config test after previous comment but before your last one. I'll open a new issue if it persists after this and leave this one less cluttered and not spoil @mariamatthews original report (sorry for the hijack).

TuxInvader commented 2 years ago

Hi @mariamatthews - All the examples should be updated now to use js_import.

Thanks

mariamatthews commented 2 years ago

Thank you @TuxInvader & @cookiemonsteruk 💕 You guys rock :)