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: make symlinks in `/etc` relative (except `/etc/static`) #54980

Merged
merged 4 commits into from Feb 21, 2019

Conversation

Projects
None yet
8 participants
@danbst
Copy link
Contributor

commented Jan 31, 2019

Motivation for this change

Main motivation is to fix extra-container, so it can run declarative NixOS containers on non-NixOS (erikarvstedt/extra-container#1 (comment)).

But seriously, there is a real problem with symlinking stuff currently. Let me show an example. Here's a simple container, where we override NixOS version.

  containers.custom = {
    autoStart = true;
    config = {
      system.nixos.version = "CUSTOM";
    };
  };

Now let's see, if that works:

### good
$ sudo nixos-container run custom -- cat /etc/os-release | grep VERSION=
VERSION="CUSTOM (Koi)"

### bad
$ sudo cat /var/lib/containers/custom/etc/os-release | grep VERSION=
VERSION="19.03.git.0652f62 (Koi)"

Indeed, in second case /var/lib/containers/custom/etc/os-release points to /etc/static/os-release of the host system, not container! This is wrong, but even worse happens on non-NixOS, where there is no /etc/static on the host - container can be created and started, but can't be rebooted.

This builds on top of #35364, however, I am not fixing the original problem (allow machinectl pull-tar to run NixOS containers). I'll make a separate PR, which builds on top of this (and #35364) and makes /etc relative symlink for container tarballs.

cc @edolstra
cc @florianjacob as original author
cc @Ekleog @7c6f434c @erikarvstedt as potential interested parties

Things done

This makes symlinks relative in host's /etc too. Rebuild-switch is handled correctly, and my machine survives multiple reboots. Also, simple installer NixOS test passes.

  • 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 nox --run "nox-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.

florianjacob and others added some commits Nov 13, 2018

nixos/etc: Make symlinks relative instead of absolute
so that the links can be followed if the NixOS installation is not mounted as filesystem root.
In particular, this makes /etc/os-release adhere to the standard:
https://www.freedesktop.org/software/systemd/man/os-release.html
Fixes #28833.

@danbst danbst requested a review from Infinisil as a code owner Jan 31, 2019

danbst added some commits Jan 31, 2019

@7c6f434c

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

Looks good to me.

Not immediately useful to me as 1) I want read-only /etc with symlinks to mutable stuff and atomic switching — and I have it — and this change is only useful as long as you have /etc/static because of not fully atomic /etc; 2) I don't want to follow some of the systemd rules — and I don't run systemd — so I will only use NixOS containers once it is clear how to run them with systemd only in the container (I am OK with systemd, DBus and PulseAudio as long as I have full control over scoping)

@7c6f434c 7c6f434c merged commit 0b91fa4 into NixOS:master Feb 21, 2019

10 checks passed

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-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
@eadwu

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

Pretty sure it's this PR, but after upgrading this morning, I've been getting boot warnings on stage-2-init about symlinks, though it doesn't seem to break anything.

One such example:

Feb 22 08:09:38 nixos unknown: booting system configuration /nix/store/gsaml9lnkc0lg87253rkyhwjb3074jjj-nixos-system-nixos-19.03.git.ea44b55
Feb 22 08:09:38 nixos stage-2-init: running activation script...
Feb 22 08:09:38 nixos stage-2-init: setting up /etc...
Feb 22 08:09:38 nixos stage-2-init: /etc/fonts directory contains user files. Symlinking may fail. at /nix/store/amhpx347b7n6m2bq24siw0qq27w3np8n-setup-etc.pl line 114.
Feb 22 08:09:38 nixos stage-2-init: /etc/dbus-1 directory contains user files. Symlinking may fail. at /nix/store/amhpx347b7n6m2bq24siw0qq27w3np8n-setup-etc.pl line 114.
Feb 22 08:09:38 nixos stage-2-init: /etc/terminfo directory contains user files. Symlinking may fail. at /nix/store/amhpx347b7n6m2bq24siw0qq27w3np8n-setup-etc.pl line 114.
Feb 22 08:09:38 nixos stage-2-init: /etc/cups directory contains user files. Symlinking may fail. at /nix/store/amhpx347b7n6m2bq24siw0qq27w3np8n-setup-etc.pl line 114.
Feb 22 08:09:38 nixos stage-2-init: /etc/zoneinfo directory contains user files. Symlinking may fail. at /nix/store/amhpx347b7n6m2bq24siw0qq27w3np8n-setup-etc.pl line 114.
Feb 22 08:09:38 nixos stage-2-init: /etc/kbd directory contains user files. Symlinking may fail. at /nix/store/amhpx347b7n6m2bq24siw0qq27w3np8n-setup-etc.pl line 114.
Feb 22 08:09:38 nixos stage-2-init: /etc/systemd/system directory contains user files. Symlinking may fail. at /nix/store/amhpx347b7n6m2bq24siw0qq27w3np8n-setup-etc.pl line 114.
Feb 22 08:09:38 nixos stage-2-init: /etc/systemd/user directory contains user files. Symlinking may fail. at /nix/store/amhpx347b7n6m2bq24siw0qq27w3np8n-setup-etc.pl line 114.
Feb 22 08:09:38 nixos stage-2-init: /etc/systemd/system-generators directory contains user files. Symlinking may fail. at /nix/store/amhpx347b7n6m2bq24siw0qq27w3np8n-setup-etc.pl line 114.
Feb 22 08:09:38 nixos stage-2-init: /etc/udev/rules.d directory contains user files. Symlinking may fail. at /nix/store/amhpx347b7n6m2bq24siw0qq27w3np8n-setup-etc.pl line 114.
@danbst

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

@eadwu I'll take a look

@dtzWill

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

Same warnings here. Basically the check for isStatic is incomplete and doesn't handle things like:

/etc/plymouth/plugins -> ../static/plymouth/plugins

Some of this can be fixed but I don't know if there's a great answer for this that doesn't involve manually walking the filesystem or making some assumptions (perhaps enabled by constructing /etc to make this easier?).

Well nevermind, I was puttering late at night so maybe there's a good answer here :). But I think this should be fixed soon and carefully. And if this is going to 19.03 resolving these warnings should be a blocker so as to not confuse or mislead users. It looks like /etc is still populated appropriately, but would be good to have some confidence we're not going to nuke user files by misunderstanding their origin or something.

@danbst

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2019

@dtzWill you were correct.

I've spent a few hours trying to make this coversion /etc/udev/rules.d -> ../static/udev/rules.d -> /etc/static/udev/rules.d, but failed. Perl explicitly warns that

/etc/udev/../static/udev/rules.d

isn't same as

/etc/static/udev/rules.d

and none of it's primitives resolve .. without taking symlink realpath. And realpath doesn't work, because /etc/static is itself tree of symlinks, so /etc/static realpath and /etc/static/udev/rules.d realpath are different.

I tend to reverting this PR, until I can figure out what is the most appropriate way to fix this. :(

dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Feb 23, 2019

Revert "Merge pull request NixOS#54980 from danbst/etc-relative"
This reverts commit 0b91fa4, reversing
changes made to 183919a.
@dtzWill

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

matix2267 added a commit to matix2267/nixpkgs that referenced this pull request Feb 27, 2019

Revert "Merge pull request NixOS#54980 from danbst/etc-relative"
This reverts commit 0b91fa4, reversing
changes made to 183919a.
@nixos-discourse

This comment has been minimized.

Copy link

commented Feb 28, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/nixos-rebuild-switch-upgrade-error-directory-contains-user-files-symlinking-may-fail/2266/2

danbst added a commit that referenced this pull request Feb 28, 2019

Revert "Merge pull request #54980 from danbst/etc-relative" (#56507)
This reverts commit 0b91fa4, reversing
changes made to 183919a.

7c6f434c added a commit that referenced this pull request Feb 28, 2019

Revert "Merge pull request #54980 from danbst/etc-relative"
This reverts commit 0b91fa4, reversing
changes made to 183919a.
@layus

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

@danbst It turns out that checking if the symlink resolves to something inside /etc/static is indeed difficult. I think that you need to perform symlink resolution, just like the kernel does. The algorithm is not trivial to implement, but my rust (and python) versions are in http://github.com/layus/readlinks.

Reimplementing this in perl will be error prone. My own implementation is still buggy.

@danbst

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

@layus there is also namei util, which I thought would be useful to reuse.

This has to be solved in some other way. Currently "static" files are checked by being symlink to /etc/static, but they can also be checked if those exist in some file etc-static-list. So etc-builder should build list of static files, which it can then use to remove unreferenced files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.