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/postgresql: support 0750 for data directory #65245

Merged
merged 8 commits into from Feb 14, 2020

Conversation

@danbst
Copy link
Contributor

danbst commented Jul 22, 2019

This is rework of part of #46670.
My usecase was to be able to inspect PG datadir as wheel user.

PG11 now allows starting server with 0750 mask for data dir.
groupAccess = true now does this automatically. The only thing you have to do
is to set group ownership.

For PG10 and below, I've described a hack how this can be done. Before this PR
hack was impossible. The hack isn't ideal, because there is short
period of time when dir mode is 0700, so I didn't want to make it official.

Test/example is present too.

@danbst danbst requested review from thoughtpolice, marsam and domenkozar Jul 22, 2019
@danbst

This comment has been minimized.

Copy link
Contributor Author

danbst commented Jul 22, 2019

Does it look fine to mention hack in docs in option description?

danbst added 2 commits Jul 21, 2019
This is rework of part of #46670.
My usecase was to be able to inspect PG datadir as wheel user.

PG11 now allows starting server with 0750 mask for data dir.
`groupAccess = true` now does this automatically. The only thing you have to do
is to set group ownership.

For PG10 and below, I've described a hack how this can be done. Before this PR
hack was impossible. The hack isn't ideal, because there is short
period of time when dir mode is 0700, so I didn't want to make it official.

Test/example is present too.
Closes #18829

+ some cleanups
@danbst danbst force-pushed the danbst:postgresql_group branch from bfac8e0 to 7e4e37f Jul 23, 2019
@danbst

This comment has been minimized.

Copy link
Contributor Author

danbst commented Jul 23, 2019

Added a commit to fix #18829

@danbst

This comment has been minimized.

Copy link
Contributor Author

danbst commented Jul 23, 2019

requesting also review from @pacien

groupAccess = mkOption {
type = types.bool;
default = false;
example = true;

This comment has been minimized.

Copy link
@pacien

pacien Jul 24, 2019

Contributor

Nitpick: unnecessary example for boolean option.

description = ''
Allow read access for group (0750 mask for data directory).
Supported only for PostgreSQL 11+. PostgreSQL 10 and lower doesn't
support starting server with 0750 mask, but a workaround like

This comment has been minimized.

Copy link
@pacien

pacien Jul 24, 2019

Contributor

IMHO, we shouldn't encourage such hacks in the official documentation.
This workaround for PostgreSQL <= 10 should probably go on the wiki instead.

else "/var/db/postgresql");

services.postgresql.initdbFlags =
mkDefault (lib.optional cfg.groupAccess "--allow-group-access");

This comment has been minimized.

Copy link
@pacien

pacien Jul 24, 2019

Contributor

Setting both groupAccess = true and initdbFlags = [ "--some-other-flag" ] causes "--allow-group" not to be added due to mkDefault.

It's simpler and more explicit to combine arguments when calling initdb in script than making the latter option depend on the former.

This comment has been minimized.

Copy link
@danbst

danbst Jul 24, 2019

Author Contributor

Indeed, thanks! I really shoud use mkBefore instead:

# nix repl ./lib
nix-repl> (evalModules { modules = [ 
                   { options.bla = mkOption { type = with types; listOf int; }; 
                     config.bla = mkBefore [1]; 
                   } 
                   { bla = [2];  } 
               ]; }).config.bla
[ 1 2 ]

Will change test.

fi
''; # */

script =
''
# Initialise the database.
if ! test -e ${cfg.dataDir}/PG_VERSION; then
initdb -U ${cfg.superUser}
initdb -U ${cfg.superUser} ${lib.concatStringsSep " " cfg.initdbFlags}

This comment has been minimized.

Copy link
@pacien

pacien Jul 24, 2019

Contributor

Combining flags here is more explicit.
Nitpick: lib. is also not necessary.

Suggested change
initdb -U ${cfg.superUser} ${lib.concatStringsSep " " cfg.initdbFlags}
initdb -U ${cfg.superUser} \
${optionalString cfg.groupAccess "--allow-group-access"} \
${concatStringsSep " " cfg.initdbFlags}

This comment has been minimized.

Copy link
@danbst

danbst Jul 24, 2019

Author Contributor

I see your point. I'll stick to module-system style though, it's not that bad.

I'll remove lib.

$machine->succeed("echo select 1 | sudo -u postgres psql"); # works after restart
$machine->succeed("sudo -u backup ls ${dataDir}");
# This tests a hack for PG <11: restore permissions to 0700 just before PG starts

This comment has been minimized.

Copy link
@pacien

pacien Jul 24, 2019

Contributor

I'm not sure whether we have to test this workaround.

@@ -306,8 +346,9 @@ in
ln -sfn "${pkgs.writeText "recovery.conf" cfg.recoveryConfig}" \
"${cfg.dataDir}/recovery.conf"
''}
chmod ${dirMode} "${cfg.dataDir}"

This comment has been minimized.

Copy link
@pacien

pacien Jul 24, 2019

Contributor

Is it necessary to re-apply the mode at each startup?
If so, this should perhaps be applied recursively.
This might be a costly operation on big databases though.

This comment has been minimized.

Copy link
@danbst

danbst Jul 24, 2019

Author Contributor

PostgreSQL is sensitive only for group mode of dataDir itself. Files inside can be even world readable, but top-level dataDir may be only 0700 or 0750 (for pg11). Otherwise it just doesn't start.

This comment has been minimized.

Copy link
@danbst

danbst Jul 24, 2019

Author Contributor

@pacien ok, I made it optional. Now groupAccess is tristate with null being "don't change mask" (previous behavior).

This comment has been minimized.

Copy link
@pacien

pacien Jul 25, 2019

Contributor

How does PostgreSQL apply the right permissions to new files within the data directory?
Is it copying the mode of dataDir itself?

danbst added 3 commits Jul 24, 2019
Making mask either 0700 or 0750 is too restrictive..
machine = { config, ...}:
{
services.postgresql.enable = true;
services.postgresql.package = pkgs.postgresql_11;

This comment has been minimized.

Copy link
@spinus

spinus Aug 12, 2019

Member

why not to make test for all the versions with generator?

This comment has been minimized.

Copy link
@danbst

danbst Aug 12, 2019

Author Contributor

This feature isn't supported for PG < 11. So not sure there is need to test it for 11, 12, 13 only (last two not yet released).

@@ -69,5 +69,47 @@ let
in
(mapAttrs' (name: package: { inherit name; value=make-postgresql-test name package false;}) postgresql-versions) // {
postgresql_11-backup-all = make-postgresql-test "postgresql_11-backup-all" postgresql-versions.postgresql_11 true;

postgresql_dirmode_change =

This comment has been minimized.

Copy link
@spinus

spinus Aug 12, 2019

Member

+1 for updating tests

This comment has been minimized.

Copy link
@danbst

danbst Feb 14, 2020

Author Contributor

I had to remove it, as I don't want to support old pgs, and new ones work fine.

danbst added 3 commits Aug 13, 2019
The only fix is to change mode to 0700 before start, because otherwise postgresql
doesn't start, and error is non-obvious.
@danbst

This comment has been minimized.

Copy link
Contributor Author

danbst commented Feb 14, 2020

I've rethought my approach. Let's not support pre11 versions THAT much. It is enough to just ensure state dir has 0700 mode for old PG.

So now dir mode is enforced only when PG is older than 11. This fixes a nasty issue when you add tmpfiles rule to make PG group-accessible and suddenly on next reboot PG doesn't start (yes, I had this...).

Now all 3 examples below start postgres, and last are possible ways to set group mode.

{
  documentation.nixos.enable = false;

  containers.test1.config = {config, pkgs, ...}: 
        {
          services.postgresql.enable = true;
          services.postgresql.port = 10000;
          services.postgresql.package = pkgs.postgresql_10;
          systemd.tmpfiles.rules = [ ''d "${config.services.postgresql.dataDir}" 0750 - - -''  ];
        };
  containers.test2.config = {config, ...}: 
        {
          services.postgresql.enable = true;
          services.postgresql.port = 10000;
          systemd.tmpfiles.rules = [ ''d "${config.services.postgresql.dataDir}" 0750 - - -''  ];
        };
  containers.test3.config = {config, ...}: 
        {
          services.postgresql.enable = true;
          services.postgresql.port = 10001;
          services.postgresql.initdbArgs = [ "--allow-group-access" ];
        };
}
@danbst danbst merged commit 5443eee into NixOS:master Feb 14, 2020
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
postgresql on aarch64-linux Success
Details
postgresql on x86_64-linux Success
Details
@danbst danbst deleted the danbst:postgresql_group branch Feb 14, 2020
@danbst

This comment has been minimized.

Copy link
Contributor Author

danbst commented Feb 14, 2020

urgh. Github UI discarded my perfect squash-commit message...

default = [];
example = [ "--data-checksums" "--allow-group-access" ];
description = ''
Additional arguments passed to <literal>initdb<literal> during data dir

This comment has been minimized.

Copy link
@Ma27

Ma27 Feb 15, 2020

Member

For the record, this breaks the manual build: 6c63107

This comment has been minimized.

Copy link
@danbst

danbst Feb 15, 2020

Author Contributor

:-( thanks for the fix

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

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