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.

Sign up for GitHub

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

Jump to bottom

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
@emilazy
pebble: v2.2.2 -> v2.3.0
6285d5e 
Also add myself to maintainers and correct meta.homepage.
@emilazy
nixos/tests/acme: don't restrict to x86_64
352e30d 
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.
@emilazy @lukateras
nixos/tests/acme: use *.test domains
d0f04c1 
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>
@emilazy
nixos/tests/acme: use CAP_NET_BIND_SERVICE
695fd78
@emilazy
nixos/tests/common/acme: enable Pebble strict mode
e6d5e83 
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.
@emilazy
nixos/tests/common/acme: don't set nameservers for client
21f183a 
The resolver is mainly useful for the ACME server, and acme.nix uses its
own DNS server to test DNS-01 challenges.
@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Apr 18, 2020
@flokli flokli requested review from flokli and arianvp April 19, 2020 09:24
@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.

@emilazy emilazy deleted the acme-test-cleanups branch April 19, 2020 22:20
This was referenced Apr 19, 2020
Merged
Closed
This was referenced Apr 21, 2020
Merged
Closed
@flokli
Copy link
Contributor

flokli commented May 2, 2020 •
edited
Loading

@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
Reviewers

@flokli flokli flokli approved these changes

@arianvp arianvp Awaiting requested review from arianvp

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@emilazy @flokli