Allow excluding xattrs at restore time by tesshuflower · Pull Request #5129 · restic/restic

tesshuflower · 2024-11-04T18:30:40Z

also allows including xattrs
default if no flags are passed include only xattrs in the user namespace (xattrs starting with user.*)

What does this PR change? What problem does it solve?

This introduces new command line flags for restic restore: --exclude-xattr and --include-xattr. These are mutually exclusive, similar to --exclude and --include.

Was the change previously discussed in an issue or on the forum?

Fixes #5089

Checklist

I have read the contribution guidelines.
I have enabled maintainer edits.
I have added tests for all code changes.
I have added documentation for relevant changes (in the manual).
There's a new file in changelog/unreleased/ that describes the changes for our users (see template).
I have run gofmt on the code in all commits.
All commit messages are formatted in the same style as the other commits in the repo.
I'm done! This pull request is ready for review.

cmd/restic/cmd_restore.go

internal/restorer/restorer.go

internal/fs/node_xattr_all_test.go

internal/fs/node_xattr.go

MichaelEischer

The changes look good so far. To get this over the finishing line the PR definitely needs a changelog entry and ideally also a short section in the restorer documentation.

internal/fs/node_aix.go

internal/fs/node_xattr_all_test.go

cmd/restic/cmd_restore.go

tesshuflower · 2024-12-03T18:41:07Z

@MichaelEischer Thanks very much for your review - I've had to rebase to get the latest, hopefully I did everything correctly. Latest updates

add unit test for xattr select filter
add changelog entry
add info in restore doc about the new flags
set default (for Linux only) to restore only user.* xattrs

For the last one is this ok? Let me know if you would prefer to keep the default of restoring all xattrs for Linux as well.

For: restic#5089 Signed-off-by: Tesshu Flower <tflower@redhat.com>

- also other platforms - move xattr include/exclude filter parsing into separate func Signed-off-by: Tesshu Flower <tflower@redhat.com>

Signed-off-by: Tesshu Flower <tflower@redhat.com>

* On Linux restore only user.* xattrs by default * restore all for other OSs * Update docs and changelog about the new restore flags --exclude-xattr and --include-xattr Signed-off-by: Tesshu Flower <tflower@redhat.com>

Signed-off-by: Tesshu Flower <tflower@redhat.com>

MichaelEischer

Sorry for the long review delay. I found a few further small nits, but besides that the PR is ready. Although you'll have to rebase one last time 🙈 .

changelog/unreleased/issue-5089

MichaelEischer · 2025-01-13T20:36:39Z

cmd/restic/cmd_restore.go


-	// no includes or excludes, set default of including all xattrs
+	// User has not specified any xattr includes or excludes
+	if runtime.GOOS == "linux" {


I've thought a bit more about this and by now I think that keeping the old behavior of restoring all xattrs by default makes more sense. Users expect that restic restores POSIX ACLs and per-file capabilities by default. Those are part of the system.* namespace, so they shouldn't be excluded by default. So let's just include all xattrs on all OS by default.

Thanks, that makes sense to me - I've also updated the docs to indicate all xattrs are restored by default.

Maybe a default exclude during restore only and backup all since the issue identified 'security.*', 'trusted.*', and 'system.*' as problematic on restore? Seems safer to me. No issue reading the security.* xattr from testing, just a problem on restore.

If the exclude default is on restore than at least it is still in the repo from the backup if needed.

I fully admit to not knowing enough about SELinux works to say with any confidence other than a suggestion to consider for someone more knowledgeable.

Sorry to just come out of nowhere, I recently ran into this same issue on OpenShift with restic.

Backup always includes all xattrs. This won't change with this PR.

I fully admit to not knowing enough about SELinux works to say with any confidence other than a suggestion to consider for someone more knowledgeable.

I don't know enough about SELinux either to clarify the exact conditions that are necessary to write the selinux xattrs. As such making a backwards incompatible change without proper understanding sounds like the wrong approach to me. If someone can provide clarity here, we can discuss for example whether it would make sense to exclude the SELinux xattr by default. Feel free to open a follow-up issue for that.

Maybe a default exclude during restore only and backup all since the issue identified 'security.*', 'trusted.*', and 'system.*' as problematic on restore?

Most if not all of those xattrs can be restored by root without issues. And I'm rather sure that some of them can even be written be regular users (in particular posix acls).

doc/050_restore.rst

MichaelEischer

LGTM. Thanks!

tesshuflower · 2025-01-14T22:06:34Z

@MichaelEischer sorry I think the failed test is because I added a random xattr (not in one of the supported linux namespaces) into one of the tests at one point and forgot to remove it. I pushed another commit to roll that part back.

MichaelEischer · 2025-01-18T21:53:05Z

The test on Windows still fails.

--- FAIL: TestOverwriteXattrWithSelectFilter (0.00s)
node_xattr_all_test.go:79: xattr user.foo not restored

setAndVerifyXattrWithSelectFilter only converts the name in attrs to uppercase but not in testAttr.

tesshuflower · 2025-01-20T15:12:50Z

The test on Windows still fails.

--- FAIL: TestOverwriteXattrWithSelectFilter (0.00s)
node_xattr_all_test.go:79: xattr user.foo not restored

setAndVerifyXattrWithSelectFilter only converts the name in attrs to uppercase but not in testAttr.

Thanks for fixing that and getting it merged! 🥳

tesshuflower mentioned this pull request Nov 4, 2024

Allow to exclude xattrs (or specific xattr namespaces) when restoring #5089

Closed

tesshuflower force-pushed the 5089_exclude_xattrs_on_restore branch from 0b9b66c to 82b3562 Compare November 4, 2024 18:43

MichaelEischer reviewed Nov 9, 2024

View reviewed changes

MichaelEischer requested changes Dec 1, 2024

View reviewed changes

internal/fs/node_aix.go Outdated Show resolved Hide resolved

internal/fs/node_xattr_all_test.go Show resolved Hide resolved

cmd/restic/cmd_restore.go Outdated Show resolved Hide resolved

cmd/restic/cmd_restore.go Outdated Show resolved Hide resolved

tesshuflower force-pushed the 5089_exclude_xattrs_on_restore branch from ebf2305 to 00212fa Compare December 3, 2024 18:37

tesshuflower marked this pull request as ready for review December 3, 2024 18:42

tesshuflower added 6 commits January 10, 2025 15:13

restore: exclude/include xattrs

af839f9

For: restic#5089 Signed-off-by: Tesshu Flower <tflower@redhat.com>

update nodeRestoreExtendedAttributes() for win

f457b16

- also other platforms - move xattr include/exclude filter parsing into separate func Signed-off-by: Tesshu Flower <tflower@redhat.com>

restore: xattr restore filter tests

24422e2

Signed-off-by: Tesshu Flower <tflower@redhat.com>

linux default restore only user xattrs, doc update

3ac697d

* On Linux restore only user.* xattrs by default * restore all for other OSs * Update docs and changelog about the new restore flags --exclude-xattr and --include-xattr Signed-off-by: Tesshu Flower <tflower@redhat.com>

xattrs - restore all by default, doc/chglog update

cd84fe0

Signed-off-by: Tesshu Flower <tflower@redhat.com>

remove bad test xattr

44cef25

Signed-off-by: Tesshu Flower <tflower@redhat.com>

MichaelEischer requested changes Jan 13, 2025

View reviewed changes

tesshuflower force-pushed the 5089_exclude_xattrs_on_restore branch from 00212fa to cd84fe0 Compare January 14, 2025 16:23

MichaelEischer approved these changes Jan 14, 2025

View reviewed changes

MichaelEischer enabled auto-merge January 14, 2025 17:55

auto-merge was automatically disabled January 14, 2025 21:42
Head branch was pushed to by a user without write access

MichaelEischer enabled auto-merge January 18, 2025 21:29

restore: fix xattr filter test on windows

5e8654c

MichaelEischer force-pushed the 5089_exclude_xattrs_on_restore branch from a9f06d1 to 5e8654c Compare January 18, 2025 22:07

MichaelEischer merged commit 8b63e1c into restic:master Jan 18, 2025

tesshuflower deleted the 5089_exclude_xattrs_on_restore branch January 20, 2025 15:12

BrewTestBot mentioned this pull request Mar 27, 2025

restic 0.18.0 Homebrew/homebrew-core#216856

Merged

Conversation

tesshuflower commented Nov 4, 2024 • edited by MichaelEischer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR change? What problem does it solve?

Was the change previously discussed in an issue or on the forum?

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaelEischer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tesshuflower commented Dec 3, 2024

Uh oh!

MichaelEischer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MichaelEischer Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tesshuflower Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

msfrucht Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelEischer Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MichaelEischer left a comment

Choose a reason for hiding this comment

Uh oh!

tesshuflower commented Jan 14, 2025

Uh oh!

MichaelEischer commented Jan 18, 2025

Uh oh!

tesshuflower commented Jan 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tesshuflower commented Nov 4, 2024 •

edited by MichaelEischer

Loading

MichaelEischer Jan 13, 2025 •

edited

Loading

msfrucht Jan 14, 2025 •

edited

Loading