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
Add matrix corporal #93777
Add matrix corporal #93777
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some comments I hope you find useful. Note that I don't know this software so if I've said anything entirely off base please feel free to correct or ignore me. If you have any questions please don't hesitate to ask.
@@ -1792,6 +1792,12 @@ | |||
githubId = 245394; | |||
name = "Hannu Hartikainen"; | |||
}; | |||
dandellion = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have this in a separate commit.
default = pkgs.matrix-corporal; | ||
}; | ||
|
||
settings = lib.mkOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every option needs a type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that would be lib.types.attrs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends what you need out of the file. That might be a good fit, or type = with types; attrsOf (oneOf [ bool int str ]);
. Or better yet take a look at #75584.
cfg = config.services.matrix-corporal; | ||
in | ||
{ | ||
options.services.matrix-corporal = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add some options (like authSharedSecretFile
) for secrets so they aren't placed in settings
and end up in the nix store.
If you do this you can have a preStart
script to replace the secrets with jq
at runtime and keep your secrets safe.
Let me know if you need any details on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, hmm that is pretty tricky, especially the AuthorizationBearerToken
stuff that can be part of the config file but aren't necessarily.
I'm actually not sure how to deal with that in a sensible way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the option type = with types; nullOr path;
and set default = null;
. From here you add to the systemd
service:
systemd.services.matrix-corporal = {
...
preStart = optionalString (cfg.authorizationBearerTokenFile) ''
use jq to insert AuthorizationBearerToken: 'cat ${cfg.authorizationBearerTokenFile}' into a new copy of the file under /var/lib/matrix-corporal
'';
};
Or some variation of that.
This wasn't really ready for review, but I still appreciate it very much! I have fixed some of the things brought up! I will fix the commits properly once this is closer to being mergeable. For now I don't want to lose the review comments by a force push 😅 |
#75584 has been merged and makes creating { lib, config, pkgs, ... }:
let
format = pkgs.formats.json {};
in {
options.settings = lib.mkOption {
type = format.type;
default = {};
};
config = {
settings.foo = 10;
settings.bar.baz = "Hello";
environment.etc.test.source = format.generate "config.json" config.settings;
};
} I would recommend using this approach here. If interested and you have any questions, please let me know. |
8b214a2
to
5f926dc
Compare
Thanks! I've already mostly used the settings options from the RFC but I've now changed the file generator and types to match the new format goodness! |
5f926dc
to
e5c73ac
Compare
e5c73ac
to
2b5ffca
Compare
Doesn't seem ready to review just yet (current version wasn't tested), I'll wait until it's not a draft anymore for a review |
@infinisil Yeah, sorry I'll refrain from opening a PR before it's ready for review next time. I thought the Draft status would be enough to not bother anyone :F @aanderse I think I need some help in figuring out the secrets stuff. I'm not entirely sure how I can use DynamicUser while also reading files with correct permissions and putting them in the right places. With the current state of the module, /var/lib/matrix-corporal isn't created, and I'm not sure why |
@dali99 I pinged @infinisil so not your fault. Feel free to open draft PRs whenever you want, it won't bother anyone. You specified |
No worries about pinging even when not ready for review yet btw, I'd love to review once it's ready and am glad @aanderse pinged me :) |
Motivation for this change
matrix-corporal is a proxy and reconciliator for matrix-synapse letting you allow/deny API endpoints, and automatically join users to rooms/communities according to a configuration policy
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)