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

[WIP] matrix-appservice-irc: init at 0.12.0 #62864

Closed
wants to merge 1 commit into from

Conversation

@pacien
Copy link
Contributor

@pacien pacien commented Jun 8, 2019

Motivation for this change

This PR introduces a package and a module for matrix-appservice-irc.

Configuration files are generated from attribute sets (cf NixOS/rfcs#42).

Blockers
  • Removal of app service registration (to protect the AS' token)
  • Serialisation of the config attribute set which contains mkOverride values (#63589 (comment))
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@pacien
Copy link
Contributor Author

@pacien pacien commented Jun 8, 2019

CC: @Ma27 you might be interested by this PR as well ;)

@pacien pacien force-pushed the pacien:matrix-appservice-irc-init branch from e2b29cd to 87b9536 Jun 8, 2019
@pacien pacien requested a review from Infinisil as a code owner Jun 8, 2019
@Ma27
Copy link
Member

@Ma27 Ma27 commented Jun 8, 2019

Awesome! I'll probably have sufficient time this weekend to actually try this out 👍

Regarding the implementation: I wonder if we want to generalize the code here as well. We currently have with this one at least four pending PRs for Matrix Bridges and all of them work pretty similar: #60151, #59211 and #62744. But this is probably out of scope and should be done in a different PR as soon as some of those are merged.

On a side note: In case you want to package the Slack integration for Matrix as well, you might want to have a look at the following stuff I've started a while ago:

@pacien
Copy link
Contributor Author

@pacien pacien commented Jun 8, 2019

@Ma27 wrote:

Regarding the implementation: I wonder if we want to generalize the code here as well. We currently have with this one at least four pending PRs for Matrix Bridges and all of them work pretty similar: #60151, #59211 and #62744. But this is probably out of scope and should be done in a different PR as soon as some of those are merged.

I don't think that we would gain much from factorising those modules.
There's not much in common between all of those, all the bridges having their particularities, being written in different languages, requiring some particular preStart step for some, having different default configuration keys to merge, environment fixes, etc. Adding a layer here might add more complexity and hurt flexibility instead.

That said, I think that we should find a way to decouple those application services from the Matrix homeserver implementation, as new official and third party homeservers may arrive sooner or later (Dendrite, Ruma, Construct to name a few).
We could define a global registry attribute to replace services.matrix-synapse.app_service_config_files and make the Synapse module pull the registration files from it. This would have the downside of only allowing one homeserver on a system though.
We also need to make sure that it's possible to override the homeserver service defined in systemd.*appservice*.{wants,after} in each module.

On a side note: In case you want to package the Slack integration for Matrix as well, you might want to have a look at the following stuff I've started a while ago:

* https://github.com/Ma27/nixpkgs/tree/package-matrix-appservice-slack (got bitten by [matrix-org/matrix-appservice-slack#125](https://github.com/matrix-org/matrix-appservice-slack/issues/125))

* [Ma27@0b824af](https://github.com/Ma27/nixpkgs/commit/0b824affc1c73b46d74314b72664bdc06d7ccb0a) (could receive message, but was unable to send them. I didn't figure out why until now).

The Slack appservice seemed completely broken last time I've tried.
I'll see what I can do, but since I've moved my company away from Slack it's not on my priority list anymore.

@Ma27
Copy link
Member

@Ma27 Ma27 commented Jun 8, 2019

I don't think that we would gain much from factorising those modules.
There's not much in common between all of those, all the bridges having their particularities, being written in different languages, requiring some particular preStart step for some, having different default configuration keys to merge, environment fixes, etc. Adding a layer here might add more complexity and hurt flexibility instead.

I'm not sure either... I like the approach of the prometheus exporters module which did a pretty great job at generalising the integration of prometheus exporters without loosing the needed flexibility.

But as I said, this is out of scope here, I was interested in your opinion, but I'll wait with this until we have some more modules for new bridges ready :)

That said, I think that we should find a way to decouple those application services from the Matrix homeserver implementation, as new official and third party homeservers may arrive sooner or later
(Dendrite, Ruma, Construct to name a few).

Haven't looked at those yet, so I'm afraid I can't say too much about this. I guess that such a registry could contain an attribute set per HS with bridges to use and each bridge module defines which HS to use by an option, right?

The Slack appservice seemed completely broken last time I've tried.
I'll see what I can do, but since I've moved my company away from Slack it's not on my priority list anymore.

Unfortunately I had a pretty similar impression.

But no hurries here, I mainly wanted to ensure that we don't do the same work simultaneously. I don't expect you to package the slack integration, especially if it's not relevant for you anymore.

@pacien
Copy link
Contributor Author

@pacien pacien commented Jun 9, 2019

@Ma27 wrote:

I'm not sure either... I like the approach of the prometheus exporters module which did a pretty great job at generalising the integration of prometheus exporters without loosing the needed flexibility.

Now that you mention it, this approach seems nice indeed 👍

Haven't looked at those yet, so I'm afraid I can't say too much about this. I guess that such a registry could contain an attribute set per HS with bridges to use and each bridge module defines which HS to use by an option, right?

I'm not sure whether it's possible to set an attribute on a dynamic reference (passed as an option).
Now that I'm thinking about it, users could always use NixOS containers to run multiple homeservers and associate bridges with each separately on the same machine.

But no hurries here, I mainly wanted to ensure that we don't do the same work simultaneously. I don't expect you to package the slack integration, especially if it's not relevant for you anymore.

I'll let you take care of the Slack bridge :)

@Ma27
Copy link
Member

@Ma27 Ma27 commented Jun 10, 2019

@pacien before I start the testing - can you please move the package into its own directory as done for the discord bridge? It's IMHO better to move such big packages into their own directory as they tend to make the maintenance of such a big package set even harder and it's easier for me to cherry-pick this onto my nixpkgs tracking branch I use to deploy my stuff :)

@pacien pacien force-pushed the pacien:matrix-appservice-irc-init branch from 87b9536 to 963b9eb Jun 10, 2019
@pacien
Copy link
Contributor Author

@pacien pacien commented Jun 10, 2019

@Ma27, it's done.

@pacien pacien force-pushed the pacien:matrix-appservice-irc-init branch from 963b9eb to 328499d Jun 10, 2019
@pacien
Copy link
Contributor Author

@pacien pacien commented Jun 10, 2019

@Ma27 wrote:

@pacien before I start the testing - can you please move the package into its own directory as done for the discord bridge? It's IMHO better to move such big packages into their own directory as they tend to make the maintenance of such a big package set even harder and it's easier for me to cherry-pick this onto my nixpkgs tracking branch I use to deploy my stuff :)

Even though I agree that it's simpler to isolate and cherry-pick, I remember having been told to do the exact opposite for another PR: #58096 (comment)

@Ma27
Copy link
Member

@Ma27 Ma27 commented Jun 26, 2019

I see 👍

Unfortunately I lack time to spend much time hacking on my infra, I'll probably be able to go back to this at one of the next weekends. Unless somebody is faster than me and merges this, you'll probably need some more patience 😅

@pacien
Copy link
Contributor Author

@pacien pacien commented Jun 26, 2019

Here's a configuration snippet that might be useful for reviewers:

let
  ircSnapshot = builtins.fetchTarball {
    url = https://github.com/pacien/nixpkgs/archive/6e74d80d800d72cc0b90b4a42e96f67a44626003.tar.gz;
  };
  ircPkgs = import ircSnapshot { };

in {
  imports = [
    "${ircSnapshot}/nixos/modules/services/misc/matrix-appservice-irc.nix"
  ];

  nixpkgs.config.packageOverrides = super: {
    inherit (ircPkgs) matrix-appservice-irc;
  };

  services.matrix-synapse = {
    enable = true;
    app_service_config_files = [
      # generate and add appservice registration file
    ];
    # ...
  };

  services.matrix-appservice-irc = rec {
    enable = true;
    registrationFile = /etc/matrix-appservice-irc/registration.yaml;
    settings = {
      ident.enabled = false;
      homeserver = {
        url = "http://localhost:8008";
        domain = "domain.tld";
        media_url = "https://public.endpoint.domain.tld";
      };
      ircService.servers = {
        "irc.freenode.net" = {
          name = "Freenode";
          port = 6697;
          ssl = true;
          sslselfsign = false;
          sendConnectionMessages = true;
          botConfig.enabled = false;
          privateMessages = {
            enabled = true;
            federate = false;
          };
          dynamicChannels = {
            enabled = true;
            createAlias = true;
            published = false;
            joinRule = "invite";
            federate = false;
            aliasTemplate = "#_freenode_$CHANNEL";
          };
          membershipLists = {
            enabled = true;
            global = {
              ircToMatrix = { initial = true; incremental = true; };
              matrixToIrc = { initial = true; incremental = true; };
            };
          };
          matrixClients = {
            userTemplate = "@_freenode_$NICK";
            displayName = "$NICK";
          };
          ircClients = {
            nickTemplate = "$DISPLAY";
            allowNickChanges = true;
            maxClients = 30;
            idleTimeout = 172800;
            lineLimit = 10;
            userModes = "iR";
          };
        };
      };
    };
  };
}
Copy link
Member

@Ma27 Ma27 left a comment

Just deployed this onto a Matrix HS (0.99.5 in that case) and it appears to work with freenode as IRC network 👍

@pacien pacien force-pushed the pacien:matrix-appservice-irc-init branch from 328499d to b59c928 Jun 29, 2019
@pacien pacien changed the title nodePackages.matrix-appservice-irc: init at 0.12.0 matrix-appservice-irc: init at 0.12.0 Jun 29, 2019
@pacien pacien force-pushed the pacien:matrix-appservice-irc-init branch from b59c928 to 0c21fb0 Jun 29, 2019
@pacien pacien changed the title matrix-appservice-irc: init at 0.12.0 [WIP] matrix-appservice-irc: init at 0.12.0 Jul 5, 2019
@pacien pacien force-pushed the pacien:matrix-appservice-irc-init branch from 0c21fb0 to d1fa71c Jul 7, 2019
@pacien pacien mentioned this pull request Jul 29, 2019
5 of 10 tasks complete
@dasJ
Copy link
Member

@dasJ dasJ commented Jul 30, 2019

I wrote my own module just a couple of days ago, but this one looks promising as well.
For reference, this is mine, maybe you'll find some things for yours: https://gist.github.com/dasJ/405e71ebf85a6f2b92b8d6dd856db907

Some things I found useful was giving CAP_NET_BIND_SERVICE if the ident service is needed, as well as instead of defauling the database URI, always adding it into the config attrset because everybody needs to add these options anyway (but this is probably a matter of taste).
I don't see any reason for the serviceDependencies, because one could just use systemd.services.<name?>.after and wants.

Autogenerating the registration (L68) and giving it to the matrix group if it exists (L73) is probably a matter of taste.
What you should really take a look at is the systemd sandboxing stuff (L84…).
My bridge is currently running with these sandboxing options, so they should be safe to use.
I just saw I forgot ProtectSystem=strict and PrivateTmp=true because it was running as dynamic user previously (but it seems you had the same problems with that).

@pacien pacien force-pushed the pacien:matrix-appservice-irc-init branch from d1fa71c to 6e74d80 Aug 11, 2019
@pacien
Copy link
Contributor Author

@pacien pacien commented Aug 11, 2019

Some things I found useful was giving CAP_NET_BIND_SERVICE if the ident service is needed

The service is now given that capability if the ident server is enabled on a port that requires it.

as well as instead of defauling the database URI, always adding it into the config attrset because everybody needs to add these options anyway (but this is probably a matter of taste).

It's already done :)

I don't see any reason for the serviceDependencies, because one could just use systemd.services.<name?>.after and wants.

IMO this allows for greater discoverability, hinting the user when looking at the documentation.

Autogenerating the registration (L68) and giving it to the matrix group if it exists (L73) is probably a matter of taste.

This is tricky when the appservice isn't running on the same machine as the homeserver as the token would be overwritten every time anything in the configuration has changed.
Only re-generating the registration file if it doesn't already exist like in the linked gist would also be a problem if some parameter requires the registration file to be rewritten.

I'd leave the user manually manage that registration file instead as some explicit intervention would be required anyway.

What you should really take a look at is the systemd sandboxing stuff (L84…).
My bridge is currently running with these sandboxing options, so they should be safe to use.
I just saw I forgot ProtectSystem=strict and PrivateTmp=true because it was running as dynamic user previously (but it seems you had the same problems with that).

I've added those options.
I think that other modules might benefit from those as their use seems to be recommended in the systemd documentation.

@pacien
Copy link
Contributor Author

@pacien pacien commented Feb 9, 2020

matrix-appservice-irc switched to Typescript since 0.14.0, requiring re-packaging it.

Sadly, I don't have enough time to do this myself, so I'll close this pull request. Anyone interested in handling this may feel free to re-use the proposed package and module and submit a new pull request.

@pacien pacien closed this Feb 9, 2020
@ajs124
Copy link
Member

@ajs124 ajs124 commented Mar 30, 2020

We've been running 0.14.0+ for a while now. The only change I made is that we also bundle dev dependencies, as discussed in this node2nix issue.

@Ralith
Copy link
Contributor

@Ralith Ralith commented Mar 30, 2020

@ajs124, could you open a PR? If nothing else it'd be handy for other people to cherry-pick for their own purposes.

@ajs124
Copy link
Member

@ajs124 ajs124 commented Mar 31, 2020

@Ralith I would have, but turns out our module is way different from what's in here, for some reason (I could have sworn we took this, at some point in the past… was this completely rewritten?).

Also, the package we have is literally exactly what's in here, except the generate.sh says nodejs_12 instead of 10.

@dasJ
Copy link
Member

@dasJ dasJ commented Mar 31, 2020

Well I haven't taken the module from here, but I rather wrote my own without checking here first. This is why the module is almost entirely different, but it does work.

@puffnfresh
Copy link
Contributor

@puffnfresh puffnfresh commented May 20, 2020

It sounds like there's a few versions of the IRC app service. Can someone raise a PR so we can get it into nixpkgs?

@puffnfresh puffnfresh mentioned this pull request May 28, 2020
3 of 10 tasks complete
@puffnfresh
Copy link
Contributor

@puffnfresh puffnfresh commented May 28, 2020

Well I packaged it yet again. @dasJ I have included the module from the gist you posted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.