Skip to content
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

feat: use the js_periodic directive to check/issue/renew certs #38

Merged
merged 26 commits into from
Mar 13, 2024
Merged

feat: use the js_periodic directive to check/issue/renew certs #38

merged 26 commits into from
Mar 13, 2024

Conversation

zsteinkamp
Copy link
Contributor

@zsteinkamp zsteinkamp commented Aug 29, 2023

Uses the new directive js_periodic in njs >= 0.8.2 to invoke clientAutoMode(r) on regular intervals. This eliminates the need to set up a cron or use docker healthchecks.

Proposed changes

  • Use js_periodic to invoke clientAutoMode() periodically.
  • Clarify some instructions for variable use in the shared dict zone.

Checklist

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

  • I have read the CONTRIBUTING document
  • If applicable, I have added tests that prove my fix is effective or that my feature works
  • If applicable, I have checked that any relevant tests pass after adding my changes
  • I have updated any relevant documentation (README.md and CHANGELOG.md)

src/index.ts Outdated Show resolved Hide resolved
examples/nginx.conf Outdated Show resolved Hide resolved
examples/nginx.conf Outdated Show resolved Hide resolved
examples/nginx.conf Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@ivanitskiy
Copy link
Contributor

Please ensure integration tests are updated and tests both triggers "periodic" and regular HTTP request. it would require updating nginx config in integration tests

examples/nginx.conf Outdated Show resolved Hide resolved
@zsteinkamp zsteinkamp changed the title feat: use the js_periodic directive to request /acme/auto feat: use the js_periodic directive to check/issue/renew certs Jan 16, 2024
src/index.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
3. When started initially, nginx would not have certificates at all (`$njs_acme_dir`, e.g. `/etc/nginx/njs-acme/`), so we can issue a new one by sending an HTTP request to a location with the `js_content` handler:

curl -vik --resolve proxy.nginx.com:8000:127.0.0.1 http://proxy.nginx.com:8000/acme/auto
3. When started initially, nginx will not have certificates at all. If you use the [example config](examples/nginx.conf), you will need to wait one minute for the `js_periodic` directive to invoke `acme.clientAutoMode` to create the certificate.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something I don't understand: Can we have the ability to kickstart via a request as well as having the periodic process run? It seems like perhaps the js_periodic invocation could be in a normal location block that kicks it off and we could have both? Very possible that there's a situation I'm missing here but this change seems to take away some flexibility.

Should we add documentation here to show how to do it the old way?

js_content acme.clientAutoModeHTTP;

It could be that some users could prefer to invoke the check using an outside source as should in the deleted documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's an interesting idea!

So we could do:

location = /acme/auto {
  js_periodic acme.clientAutoMode 1m;
  js_content acme.clientAutoModeHTTP;
}

Then people who were impatient could just hit it with curl. Ya?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting I had not considered combining the two. Kind of strange the js_periodic needs to be in a location block to work since it's not tied to a location necessarily but 🤷 .

Yes I think this accomplishes the goal I was thinking of. Gives the user's automation the power to kickstart things on their desired cadence while still maintaining the automatic operation.
If we get requests for entirely manual management we could introduce a config option but until then this seems like a reasonable compromise 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet. I've documented it as an option in the README.md. LMK what you think...

zsteinkamp and others added 20 commits March 11, 2024 10:18
…v var. Will update README.md if this looks like a good path.
…, remove advanced endpoints from example conf
…ModeWeb to clientAutoModeHTTP; fix unused var warning
… examples; move some util functions to utils.ts
@zsteinkamp zsteinkamp marked this pull request as ready for review March 12, 2024 17:25
@zsteinkamp zsteinkamp merged commit 9306982 into nginx:main Mar 13, 2024
2 checks passed
@zsteinkamp zsteinkamp deleted the zsteinkamp/periodic branch March 13, 2024 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants