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

nixos/acme: add option to delay renewal untill after other units #221363

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

icewind1991
Copy link
Contributor

  • allow adding additional units to the after section
  • add documentation for delaying renewal untill after timesync is done
Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@benaryorg benaryorg left a comment

Choose a reason for hiding this comment

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

Reviewed points
  • changes are backward compatible
  • removed options are declared with mkRemovedOptionModule n/a
  • changes that are not backward compatible are documented in release notes n/a
  • module tests succeed on ARCHITECTURE
  • options types are appropriate
  • options description is set
  • options example is provided
  • documentation affected by the changes is updated
Possible improvements

The options example is not provided, however the ACME documentation contains a full example of how to use this mechanism.
Maybe the documentation could be linked, I don't know how though, and I think this change is sufficient as-is.

Comments

lgtm

nixos/modules/security/acme/default.md Outdated Show resolved Hide resolved
nixos/modules/security/acme/default.md Outdated Show resolved Hide resolved
nixos/modules/security/acme/default.md Outdated Show resolved Hide resolved
Copy link
Contributor

@m1cr0man m1cr0man left a comment

Choose a reason for hiding this comment

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

This looks good, however what do you think of adding the time sync dependency by default? Seems like it would be a common enough concern

@jian-lin
Copy link
Contributor

what do you think of adding the time sync dependency by default

FYI, enabling systemd-time-wait-sync.service (thus adding time-sync.target) has some side effects.

man systemd.special:

The service manager automatically adds dependencies of type After= for this target unit to all SysV init script service units with an LSB header referring to the "$time" facility, as well to all timer units with at least one OnCalendar= directive.

This target provides stricter clock accuracy guarantees than time-set.target (see above), but likely requires network communication and thus introduces unpredictable delays.

IMHO, the issue that cert renewal before time is synced after boot is not common and I never have such issue. Will a retry after time is synced fix this issue? Does our acme module have a retry mechanism? I am not sure.

@icewind1991
Copy link
Contributor Author

I expect that the only reason I ran into the issue was because the system is a raspberry pi with a non-persistent / so the clock is way out of sync at boot.

Most setups probably don't have a lot of clock drift at boot

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
- allow adding additional units to the `after` section
- add documentation for delaying renewal untill after timesync is done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants