nginx / njs-acme

Nginx NJS module runtime to work with ACME providers like Let's Encrypt for automated no-reload TLS certificate issue/renewal.
Apache License 2.0
57 stars 9 forks source link

add Logger class #9

Closed ryepup closed 1 year ago

ryepup commented 1 year ago

Proposed changes

Adds a minimal leveled logger wrapping ngx.log, intended to ensure consistent log prefixes and control log verbosity.

This lets us swap from ngx.log to Logger, diffs like

- ngx.log(ngx.INFO, `njs-acme: [client] Item has status: ${respData.status}`)
+ log.info('Item has status:', respData.status)

Also gets setup for unit tests on things in the src folder, and some config tweaks to make vscode handle formatting more automatically.

fixes #6

Checklist

Before creating a PR, run through this checklist and mark each as complete.

ryepup commented 1 year ago

if this looks good, I'll run through the other files and update loggers in another PR so @zsteinkamp can start re-using some of the unit-test stuff setup here.

ivanitskiy commented 1 year ago

please update the client.ts file to use the newly added logger instead of ngx.log: e.g. https://github.com/nginxinc/njs-acme/blob/274755f50c2ae86a6fed4fd17b359e6f30d02faa/src/client.ts#L351C7-L351C15

zsteinkamp commented 1 year ago

please update the client.ts file to use the newly added logger instead of ngx.log: https://github.com/nginxinc/njs-acme/blob/274755f50c2ae86a6fed4fd17b359e6f30d02faa/src/client.ts#L351C7-L351C15

Ryan mentioned he'd do that once we were happy with the interface/implementation. 👍

ivanitskiy commented 1 year ago

Ryan mentioned he'd do that once we were happy with the interface/implementation. 👍

I'm good with adding the logger interface as of now.

ryepup commented 1 year ago

Got all references updated

ryepup commented 1 year ago

i assume that it got tested

npm run test works, I had incorrectly assumed that was definitive. I tweaked the logger initialization to be lazier about accessing ngx to resolve a TypeError: cannot get property "INFO" of undefined error, but I'm getting some problems following the testing instructions in the README. The logger seems fine but r.variables doesn't seem to behave for me.

2023/07/06 13:39:15 [error] 98#98: *17 js: njs-acme: [utils] Variable njs_acme_server_names not found and no default value given.

Will try to get with @zsteinkamp today or tomorrow to work through it.

ivanitskiy commented 1 year ago

npm run test works, Unfortunately, there are no real tests that would test the lib functionality. at least need to spin up nginx (example config) and send an HTTP request to a handler that would issue a cert from acme. depending on wich acme provider you are using you would need your server reachable on the internet. there is a Pebble acme for testing in docker-compose.

ryepup commented 1 year ago

i assume that it got tested

Works fine if I bring in the changes from #16. Omitted that in this branch to reduce conflicts.