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

ACME test cleanups #85503

Merged
merged 6 commits into from Apr 19, 2020
Merged

ACME test cleanups #85503

merged 6 commits into from Apr 19, 2020

Conversation

emilazy
Copy link
Member

@emilazy emilazy commented Apr 18, 2020

Motivation for this change

The parts of #83474 that don't add new functionality. Should be backported to 20.03 on merge.

cc @flokli

@GrahamcOfBorg test acme

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

emilazy and others added 6 commits April 18, 2020 05:15
Also add myself to maintainers and correct meta.homepage.
This was added in aade4e5, but the
implementation of the ACME module has been entirely rewritten since
then, and the test seems to run fine on AArch64.
Shimming out the Let's Encrypt domain name to reuse client configuration
doesn't work properly (Pebble uses different endpoint URL formats), is
recommended against by upstream,[1] and is unnecessary now that the ACME
module supports specifying an ACME server. This commit changes the tests
to use the domain name acme.test instead, and renames the letsencrypt
node to acme to reflect that it has nothing to do with the ACME server
that Let's Encrypt runs. The imports are renamed for clarity:

* nixos/tests/common/{letsencrypt => acme}/{common.nix => client}
* nixos/tests/common/{letsencrypt => acme}/{default.nix => server}

The test's other domain names are also adjusted to use *.test for
consistency (and to avoid misuse of non-reserved domain names such
as standalone.com).

[1] letsencrypt/pebble#283 (comment)

Co-authored-by: Yegor Timoshenko <yegortimoshenko@riseup.net>
This lets us get early warning about any bugs or backwards-compatibility
hazards in lego.

Pebble will default to this in the future, but doesn't currently;
see https://github.com/letsencrypt/pebble/blob/v2.3.0/README.md#strict-mode.
The resolver is mainly useful for the ACME server, and acme.nix uses its
own DNS server to test DNS-01 challenges.
@flokli flokli merged commit ab0da25 into NixOS:master Apr 19, 2020
@flokli
Copy link
Contributor

flokli commented Apr 19, 2020

@emilazy I tried to git cherry-pick -x 6285d5eabda26d0e696a328bcc9b8bf33dea1b3a e6d5e83cf10f8d6d900c53f8b29399e3619434c7 352e30df8a38b1b673c19de817fedca7d3d95d71 d0f04c1623ae74f256ff5ced77ac78c7fe3b6abc 695fd78ac45763b02ae4c68abda28974bb72c96b 21f183a3fe4eddbbb418cc1ee37a9f86526f675a, but this seems to not apply cleanly. Maybe we missed another patch that landed in master?

Could you open a backport PR against 20.03 instead?

@emilazy
Copy link
Member Author

emilazy commented Apr 19, 2020

It's #81848; will open a manual backport PR.

cc @grahamc – might it be a good idea to backport the implementation of specialisation.* to 20.03, without removing nesting? Currently there's no way to write a configuration or test using the feature that's compatible with both 20.03 and master.

@flokli
Copy link
Contributor

flokli commented May 2, 2020

@emilazy This did a great work on making the acme server infrastructure reusable for other vm tests that also involve acme certificates! ❤️

Do you have any plans on moving more of the tooling into common/acme/*?

I could imagine the acme and dnsserver machine configurations being moved there entirely, and networking.nameservers = lib.mkForce [ nodes.dnsserver.config.networking.primaryIPAddress ]; also being part of ./common/acme/client.

Once the acme tooling is easy usable elsewhere, we could then stop setting enableACME to false in some other VM tests.

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.

None yet

2 participants