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/tests/dendrite: init #121777

Merged
merged 1 commit into from May 14, 2021
Merged

Conversation

mjlbach
Copy link
Contributor

@mjlbach mjlbach commented May 5, 2021

Motivation for this change

Matrix-nio is actively maintained, but also didn't support dendrite until tonight. I filed an upstream PR to fix this which was recently merged matrix-nio/matrix-nio#254. We'll need to wait until upstream cuts the next release in order to verify/merge this test (it worked on my local machine).

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.

cc @dotlambda @infinisil

@dotlambda
Copy link
Member

In light of #121728 (comment), can we please add TLS to the test with a non world-readable key? That should fail atm.

@mjlbach
Copy link
Contributor Author

mjlbach commented May 5, 2021

Yes, I can add that this weekend

@dotlambda dotlambda changed the title matrix-dendrite: add test for nixos module nixos/matrix-dendrite: add test May 5, 2021
@mjlbach
Copy link
Contributor Author

mjlbach commented May 6, 2021

Depends on #121942

@mjlbach mjlbach force-pushed the matrix_dendrite_module_test branch 2 times, most recently from cd439e1 to 22657d4 Compare May 9, 2021 15:28
@mjlbach
Copy link
Contributor Author

mjlbach commented May 9, 2021

My nixpkgs PR bumping matrix-nio is now in master, I'm cleaning up and including a couple additional features (including the TLS case). I assume the latter will fail, so I'll also add additional option we discussed on matrix to the systemd service

Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

Please substitute matrix-dendrite with dendrite (also in the filename) and add the test to nixos/tests/all-tests.nix as well as dendrite's passthru.tests.

@mjlbach mjlbach force-pushed the matrix_dendrite_module_test branch from 22657d4 to 6136ef9 Compare May 13, 2021 16:22
@mjlbach mjlbach marked this pull request as ready for review May 13, 2021 16:23
@mjlbach
Copy link
Contributor Author

mjlbach commented May 13, 2021

I went ahead and updated the test. Questions:

  1. How do I test the pastthru test? I've validated the test works with NIX_PATH=nixpkgs=$(pwd) nix-build '<nixpkgs/nixos/tests/dendrite.nix>'
  2. Will the passthru need to be special cased for darwin, as I'm not sure the test will work.

Re: hardening the module and adding a test, I'll do that in a separate PR.

@mjlbach mjlbach requested a review from dotlambda May 13, 2021 16:24
@@ -13,6 +13,8 @@ buildGoModule rec {

vendorSha256 = "1l1wydvi0yalas79cvhrqg563cvs57hg9rv6qnkw879r6smb2x1n";

passthru.tests = nixosTests.dendrite;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
passthru.tests = nixosTests.dendrite;
passthru.tests = {
inherit (nixosTests) dendrite;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason for this? The former pattern is very common in nixpkgs, and we aren't inheriting multiple tests.

Copy link
Member

Choose a reason for hiding this comment

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

It's what every other package does.

Copy link
Contributor Author

@mjlbach mjlbach May 13, 2021

Choose a reason for hiding this comment

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

This isn't true, but I've changed it to what you suggested:

pkgs/servers/web-apps/engelsystem/default.nix
40:  passthru.tests = nixosTests.engelsystem;

pkgs/servers/keycloak/default.nix
58:  passthru.tests = nixosTests.keycloak;

pkgs/servers/miniflux/default.nix
34:  passthru.tests = nixosTests.miniflux;

pkgs/servers/pinnwand/steck.nix
25:  passthru.tests = nixosTests.pinnwand;

pkgs/servers/pinnwand/default.nix
46:  passthru.tests = nixosTests.pinnwand;

pkgs/servers/nextcloud/default.nix
16:    passthru.tests = nixosTests.nextcloud;

pkgs/applications/audio/musescore/default.nix
44:  passthru.tests = nixosTests.musescore;

pkgs/applications/networking/irc/convos/default.nix
83:  passthru.tests = nixosTests.convos;

pkgs/applications/misc/keepassx/community.nix
123:  passthru.tests = nixosTests.keepassxc;

pkgs/os-specific/linux/firejail/default.nix
72:  passthru.tests = nixosTests.firejail;

pkgs/os-specific/linux/usbguard/default.nix
77:  passthru.tests = nixosTests.usbguard;

pkgs/tools/security/bitwarden_rs/vault.nix
19:  passthru.tests = nixosTests.bitwarden;

pkgs/tools/security/bitwarden_rs/default.nix
38:  passthru.tests = nixosTests.bitwarden;

Copy link
Member

Choose a reason for hiding this comment

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

You're right, but most of them do it correctly, i.e. as documented: https://nixos.org/manual/nixpkgs/unstable/#var-meta-tests.

Comment on lines 91 to 93
homeserver.succeed(
"/run/current-system/specialisation/running/bin/switch-to-configuration test >&2"
)
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied synapse, I can change it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually necessary for the test to wait until the service is started, removing it causes the test to fail with

Exception: unit "dendrite.service" is inactive and there are no pending jobs

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we have to do homeserver.wait_for_unit("network.target")?

@dotlambda
Copy link
Member

dotlambda commented May 13, 2021

How do I test the pastthru test?

nix-build -A dendrite.passthru.tests

Will the passthru need to be special cased for darwin, as I'm not sure the test will work.

No need to, ofborg only runs tests on Linux.

Re: hardening the module and adding a test, I'll do that in a separate PR.

I'm strongly opposed to that. We either fix the module now or remove it from 21.05.

@dotlambda dotlambda changed the title nixos/matrix-dendrite: add test nixos/dendrite: add test May 13, 2021
@mjlbach
Copy link
Contributor Author

mjlbach commented May 13, 2021

I'm strongly opposed to that. We either fix the module now or remove it from 21.05.

You are strongly demotivating me to contribute to nixpkgs. You should adjust how you talk to contributors, because I am losing interest in working with you.

  1. I was planning on submitting the hardening PR this weekend. This PR is adding a test. The other PR will add hardening and an associated test. It's normal to scope PRs based on the features they add.
  2. There are modules which leak secrets into the nix store. TLS certs/registration secrets are an optional component of the module.

@mjlbach mjlbach requested a review from Ma27 May 13, 2021 17:17
@dotlambda
Copy link
Member

@mjlbach I wasn't talking about hardening, sorry if it sounded like I was. I would just like to see a test using tlsCert and tlsKey, and I guess a fix for these options using systemd credentials. Hardening is a nice to have thing and can definitely be done in a separate PR.

@mjlbach
Copy link
Contributor Author

mjlbach commented May 13, 2021

As part of the hardening PR, I'm also going to change the tlsCert/key as we discussed previously using systemd credentials. I just wanted to get the test infrastructure in as I had to fix a lot of things (both upstream, and in the matrix-nio dependency chain). I don't see the harm in having a dedicated PR that enables the test, which I can then expand on when I add tests re: the tls issue.

@dotlambda
Copy link
Member

Okay, if you do it before 21.05 then it's fine with me. Please change the two things mentioned above though.

Btw, thanks a lot for going as far as fixing stuff in nio for this test!

@mjlbach mjlbach force-pushed the matrix_dendrite_module_test branch from 6136ef9 to 62785de Compare May 13, 2021 17:39

nodes = {
homeserver = { pkgs, ... }: {
specialisation.running.configuration = {
Copy link
Member

Choose a reason for hiding this comment

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

This is the culprit as it will build the configuration running but not run it. With

Suggested change
specialisation.running.configuration = {

it should work.
The option specialisation can be used for switching configuration within a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be resolved now, thanks!

@mjlbach mjlbach force-pushed the matrix_dendrite_module_test branch from 62785de to 5f00525 Compare May 14, 2021 09:03
@mjlbach mjlbach force-pushed the matrix_dendrite_module_test branch from 5f00525 to 94e89ad Compare May 14, 2021 09:06
@dotlambda dotlambda changed the title nixos/dendrite: add test nixos/tests/dendrite: init May 14, 2021
@dotlambda dotlambda merged commit 4628449 into NixOS:master May 14, 2021
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