feat(internal/fs/node): Restore ownership by name#5449
Conversation
|
I think it should be optinal. Setting it as default is breaking change. Also any chance we could get it for Windows? |
|
While I can see why you say it is a breaking change, I don't think it is one since the original behavior should stay on most systems where the uid and gid mappings are the same. It just fixes the behavior for systems where this isn't the case. I don't know about Windows, but it didn't have any chown-like thing before on a restore. Also I just don't know much about Windows in general, do they even have the concept of file ownership with uid and gid? |
|
There is concept of Security Descriptors on Windows. Support was introduced in #4509 (and this is the example why features should be optional even if they seems like good default). |
f4b1cd1 to
fed57ac
Compare
|
@zumm I added a CLI flag to optionally enable the new behavior. |
Windows will have to be handled separately. The security descriptors rely on unique IDs which can't be easily translated between systems. |
fed57ac to
cf722eb
Compare
|
Rebased to resolve conflicts |
MichaelEischer
left a comment
There was a problem hiding this comment.
While taking a closer look, I noticed a rather ugly problem that cannot be solved by this PR. Restic stores POSIX ACLs in a backup, but only in their numeric on-disk format. Thus, we have no way of restoring those based on their name.
However, fixing that is out of scope for this PR. Instead let's document the limitation somewhere (ideally in the CLI parameter). Besides that the PR only needs tests for the new codepaths.
What exactly would you write there? I feel like it does not really belong into the usage description of the new CLI flag, but maybe for the restore command itself. |
cf722eb to
daef32b
Compare
The CLI flag could get a |
daef32b to
0012c90
Compare
|
@MichaelEischer anything missing from my side or something I can do to get it merged? No pressure though, I see you are doing a lot of work for restic already! ❤️ |
|
Sorry to ping again, @MichaelEischer any chance you could have another look? I resolved all review comments. |
MichaelEischer
left a comment
There was a problem hiding this comment.
Sorry for the long delay. I only have two small nits regarding the changelog left, then this is ready for merging.
0012c90 to
8fae460
Compare
|
Done! Thanks for reviewing! |
MichaelEischer
left a comment
There was a problem hiding this comment.
LGTM.
For handling POSIX ACLs correctly, I've created a minimal issue: #5598
What does this PR change? What problem does it solve?
When doing a restore the UID and GID for the chown are now looked up on the current system, so that the ownership will actually match the expected user/group names.
I am not sure if this needs extra tests, since the existing ones all pass which at least validates that it didn't break existing behavior.
I am also not sure if this should be behind a CLI flag and if this needs additional documentation.
Please let me know if any of those things are needed, I'm happy to put more work into this feature.
This is my first PR to restic, but I've been a very happy user so far and this issues has been biting me quite a bit. I went so far to even create a script that would be put into every snapshot, in order to correctly restore the ownership, but it would be great to have a solution directly in restic.
Was the change previously discussed in an issue or on the forum?
Fixes #3572
Checklist
changelog/unreleased/that describes the changes for our users (see template).