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/paperless: Support remote databases #368137

Merged
merged 1 commit into from
Jan 19, 2025

Conversation

gefla
Copy link

@gefla gefla commented Dec 25, 2024

When the database is on another host, unit isolation for the document consumer has to be disabled.

Things done

Using this in a local setup.

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 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
  • Fits CONTRIBUTING.md.

@leona @SuperSandro2000 @erikarvstedt

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 25, 2024
@gefla
Copy link
Author

gefla commented Dec 25, 2024

@ofborg test paperless

@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 25, 2024
@ambroisie
Copy link
Contributor

Presumably this is incompatible with dabase.createLocally?

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

I won't accept a new option for this.
How about we make it conditional on the local database option added in #359563 ?

@gefla gefla force-pushed the db-is-remote branch 2 times, most recently from 32811fb to 519a7a7 Compare December 26, 2024 12:15
gefla pushed a commit to gefla/nixpkgs that referenced this pull request Dec 26, 2024
When the database is on another host, unit isolation for the document
consumer and scheduler have to be disabled. Following
NixOS#368137 (review)
making this conditional on the `database.createLocally` option.
@gefla
Copy link
Author

gefla commented Dec 26, 2024

I won't accept a new option for this. How about we make it conditional on the local database option added in #359563 ?

Updated. PTAL.

@Atemu Atemu changed the title nixos/paperless: Add database.isRemote option nixos/paperless: Support remote databases Jan 14, 2025
@Atemu
Copy link
Member

Atemu commented Jan 14, 2025

Am I missing something or does the code not reflect the intention? This only adds hardening when ran locally rather than loosen it when not local.

This also doesn't really make much sense to me; why does it need private networking? Local DB connections are made using unix sockets, not networking, right?

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 14, 2025
@SuperSandro2000
Copy link
Member

This only adds hardening when ran locally rather than loosen it when not local.

If the option is not sec, aka no private networking, then it is open to connect via TCP to other things.

why does it need private networking? Local DB connections are made using unix sockets, not networking, right?

yeah and none local DBs are done via networking.

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

If the option is not sec, aka no private networking, then it is open to connect via TCP to other things.

Right but that's not the stated intention. Currently, networking doesn't appear to be restricted at all, so what this really does is add hardening?

It's extremely confusing. The intention should be re-stated and explained in the commit message to remove this confusion.

yeah and none local DBs are done via networking.

The correct hardening to apply would be to block networking entirely then.

@gefla
Copy link
Author

gefla commented Jan 15, 2025

If the option is not sec, aka no private networking, then it is open to connect via TCP to other things.

Right but that's not the stated intention. Currently, networking doesn't appear to be restricted at all, so what this really does is add hardening?

No. If you check the default service config that's underlying all unit definitions in the module, it sets PrivateNetwork = true.

It's extremely confusing. The intention should be re-stated and explained in the commit message to remove this confusion.

yeah and none local DBs are done via networking.

The correct hardening to apply would be to block networking entirely then.

That won't work since networking is required at least for the web frontend and the task queue to function. For the scheduler and consumer to work with a remote database, they also need to disable the default PrivateNetwork = true setting inherited from defaultServiceConfig.

I believe the commit message concisely explains that. Of course I'd welcome specific proposals for a better message. Do you have any?

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

That clears it up, thanks.

You should put into the commit message a description or explanation of your changes when it's not obvious why they were done in a certain way or what the intention is.
Actually, you should do this even if you think it's obvious because reviewers aswell as future us looking at the history won't necessarily be familiar with how this specific part works, so an explanation is extremely helpful in any case.

What you wrote in reply to me is basically that; you could just reword that slightly.

nixos/modules/services/misc/paperless.nix Outdated Show resolved Hide resolved
@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Jan 15, 2025
When the database is on another host, unit isolation for the document
consumer and scheduler have to be disabled. This is currently enabled by
default via `PrivateNetwork = false` in defaultServiceConfig. Following
NixOS#368137 (review)
making this conditional on the `database.createLocally` option.
@gefla
Copy link
Author

gefla commented Jan 18, 2025

That clears it up, thanks.

You should put into the commit message a description or explanation of your changes when it's not obvious why they were done in a certain way or what the intention is. Actually, you should do this even if you think it's obvious because reviewers aswell as future us looking at the history won't necessarily be familiar with how this specific part works, so an explanation is extremely helpful in any case.

What you wrote in reply to me is basically that; you could just reword that slightly.

Reworded the commit message accordingly.

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Intention and diff LGTM.

@SuperSandro2000 SuperSandro2000 merged commit 9dee4ae into NixOS:master Jan 19, 2025
34 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants