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

nixos/imaginary: init #215222

Merged
merged 2 commits into from
Feb 12, 2023
Merged

nixos/imaginary: init #215222

merged 2 commits into from
Feb 12, 2023

Conversation

dotlambda
Copy link
Member

@dotlambda dotlambda commented Feb 8, 2023 •
edited
Loading

Tested using

services.nextcloud.config = {
  enabledPreviewProviders = [
    ''OC\Preview\Imaginary''
  ];
  preview_imaginary_url = "http://localhost:8088/";
};
services.imaginary = {
  enable = true;
  settings = {
    a = "localhost";
    p = 8088;
    return-size = true;
  };
};
Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@dotlambda dotlambda force-pushed the nixos-imaginary-init branch 2 times, most recently from 7588230 to 468d34f Compare February 8, 2023 01:27
urandom2
urandom2 approved these changes Feb 8, 2023
View reviewed changes
Copy link
Contributor

@urandom2 urandom2 left a comment

Choose a reason for hiding this comment

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

I am pleased with the derivation changes; still new to nixos modules, but nothing stands out as egregious

pkgs/servers/imaginary/default.nix Show resolved Hide resolved
pkgs/servers/imaginary/default.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot requested a review from urandom2 February 8, 2023 01:53
pennae
pennae reviewed Feb 8, 2023
View reviewed changes
nixos/modules/services/networking/imaginary.nix Outdated
]);

options = {
a = mkOption {
Copy link
Contributor

Choose a reason for hiding this comment

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

are these option names that good? we'd much prefer giving the descriptive names and amending the description with something like Corresponds to the `-a` parameter.. this wouldn't stop anyone using the original names, but it'd make module instances more readable.

(note: we're only taking issue with a and p, but return-size is totally good as is)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm also not super happy with it but I think this would be against the spirit of NixOS/rfcs#42.
However we could file a PR upstream that introduces e.g. -address as an alias for -a.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we could just add these options outside of settings.

Copy link
Contributor

Choose a reason for hiding this comment

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

both sound reasonable, but adding them outside of settings is probably better. that more clearly communicates that there could be special handling going on.

nixos/modules/services/networking/imaginary.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/imaginary.nix Outdated Show resolved Hide resolved
@dotlambda
Copy link
Member Author

I can't get ports < 1024 to work:

listen: listen tcp 10.0.0.23:99: bind: permission denied

Does someone have an idea?

@pennae
Copy link
Contributor

pennae commented Feb 8, 2023

I can't get ports < 1024 to work:

listen: listen tcp 10.0.0.23:99: bind: permission denied

Does someone have an idea?

PrivateUsers= causes the unit to be executed with no capabilities in the hosting namespace, so your capability sets do nothing. should work if you drop it.

@dotlambda dotlambda marked this pull request as ready for review February 8, 2023 06:10
@dotlambda
Copy link
Member Author

I added TemporaryFileSystem and BindReadOnlyPaths.

pennae
pennae approved these changes Feb 9, 2023
View reviewed changes
Copy link
Contributor

@pennae pennae left a comment

Choose a reason for hiding this comment

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

haven't tested the module, but it looks good.

@dotlambda dotlambda force-pushed the nixos-imaginary-init branch 2 times, most recently from 9e2235d to fbd5bec Compare February 10, 2023 02:47
@dotlambda dotlambda merged commit 7b60fce into NixOS:master Feb 12, 2023
@dotlambda dotlambda deleted the nixos-imaginary-init branch February 12, 2023 16:42
SuperSandro2000
SuperSandro2000 reviewed Feb 13, 2023
View reviewed changes
nixos/modules/services/networking/imaginary.nix

address = mkOption {
type = types.str;
default = "";
Copy link
Member

Choose a reason for hiding this comment

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

Does emptry string equal to localhost? If I understand the nextcloud doc correct it sounds like you want to run imaginary always bound on localhost.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it does mean that according to https://pkg.go.dev/net#Dial. I went with upstream's default: https://github.com/h2non/imaginary/blob/master/imaginary.go#L20

Copy link
Member Author

Choose a reason for hiding this comment

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

Never mind, it means any address. That's also what it says in https://github.com/h2non/imaginary#command-line-usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it should be changed to localhost by default. See #100192. But the problem is that localhost means 127.0.0.1 and doesn't include ::1.

Copy link
Member

Choose a reason for hiding this comment

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

Can we use [::1]?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, gotta love it. golang/go#9334

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we set address = "localhost" by default anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

if it's primarily not meant to be accessed from other hosts, probably? don't know enough about the service to be sure. but then again, if it makes for such interesting complications it might just be better to rely on the firewall existing and a note that by default all addresses are bound? 😕

Copy link
Member Author

Choose a reason for hiding this comment

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

#217339

Copy link
Member

Choose a reason for hiding this comment

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

For the nextcloud use case it is only accessed through localhost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@pennae pennae pennae approved these changes

@jtrees jtrees Awaiting requested review from jtrees

@dit7ya dit7ya Awaiting requested review from dit7ya

@urandom2 urandom2 Awaiting requested review from urandom2

@onny onny Awaiting requested review from onny

@yayayayaka yayayayaka Awaiting requested review from yayayayaka

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 11.by: package-maintainer 12.approved-by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dotlambda @pennae @urandom2 @SuperSandro2000