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
Fix dangerous forget-all operation which drops all snapshots #4568
Conversation
Hi Jakob, Thank you for working on this. I'm the original issue OP. I've never looked at restic code before, but I have a few comments, though of cause they may be misplaced, as I have only looked at this tiny part of the code.
Best Alex |
Hey Alex, thanks for the great feedback and the catched bugs! :-) I'll fix that in the next commits on this PR. But today it's too late. I'm happy to get more feedback later on! It's also my first time contributing to restic and first time using the go language ^^ In case you're not familiar to github PRs, note that you can also add comments to the code in the "Files changed" tab. |
for _, d := range []restic.Duration{opts.Within, opts.WithinHourly, opts.WithinDaily, | ||
opts.WithinMonthly, opts.WithinWeekly, opts.WithinYearly} { | ||
if d.Hours < 0 || d.Days < 0 || d.Months < 0 || d.Years < 0 { | ||
return errors.Fatal("durations containing negative values are not allowed for --keep-within*") | ||
} | ||
if d.Hours != 0 || d.Days != 0 || d.Months != 0 || d.Years != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arberg regarding 1.: these lines should be correct. d
is a Duration
which is defined as
type Duration struct {
Hours, Days, Months, Years int
}
The duration specification has no week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and each opts.Within*
option has such a duration specification as argument. That's my understanding.
allKeepXUndefined := opts.Last == 0 && opts.Hourly == 0 && opts.Daily == 0 && opts.Weekly == 0 && | ||
opts.Monthly == 0 && opts.Yearly == 0 | ||
|
||
if len(opts.KeepTags) > 0 && allKeepXUndefined && allKeepWithinXUndefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding 2.: Why do you think it doesn't cover --keep-within
? It should be covered by allKeepWithinXUndefined
which takes opts.Within
into account.
f.VarP(&forgetOptions.Within, "keep-within", "", "keep snapshots that are newer than `duration` (eg. 1y5m7d2h) relative to the latest snapshot")
Should be fine, right?
I should try to add some tests in |
01e3474
to
1248332
Compare
@@ -80,6 +81,7 @@ func TestForgetOptionValues(t *testing.T) { | |||
{ForgetOptions{WithinWeekly: restic.ParseDurationOrPanic("1y2m3d-3h")}, negDurationValErrorMsg}, | |||
{ForgetOptions{WithinMonthly: restic.ParseDurationOrPanic("-2y4m6d8h")}, negDurationValErrorMsg}, | |||
{ForgetOptions{WithinYearly: restic.ParseDurationOrPanic("2y-4m6d8h")}, negDurationValErrorMsg}, | |||
{ForgetOptions{KeepTags: []restic.TagList{[]string{"tag1"}}}, keepTagsAloneMsg}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the TagLists can be created in a more concise way? I don't know go lang.
@MichaelEischer @fd0 Do you want to review and merge or give feedback on this bugfix? Edit: Sorry, I forgot some of the checkboxes above. I'll need to rebase again, run gofmt and update the changelog. |
@schoettl Unfortunately, I won't have time to take a look at the PR before christmas, sorry. |
Happy Christmas! May I just ping on this PR? Maybe someone has time for review over the holidays :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enforcing that --keep-tag
is always used together with another keep option feels wrong. In particular, it will lead unwanted side effects if the snapshots with the specified tags are not the latest snapshots in the snapshot group. Then both the latest snapshot and the tagged snapshots in the snapshot group would be kept, which breaks the expected semantics.
I'd like to propose a different approach:
- Fix the current behavior by adding a check that
forget
refuses to work, if any snapshot group would end up with zero snapshots. This check would necessarily have to happen at runtime of the command, and cannot be checked before hand. - Add an
--unsafe-allow-remove-all
option for theforget
command. This will disable the above check. - To prevent the worst cases of fat fingering,
--unsafe-allow-remove-all
must refuse to work unless a host/path/tag filter is specified or any other--keep-*
option. This adds a new feature: users can then delete all snapshots from a specific host using e.g.restic forget --host old-host --unsafe-allow-remove-all
.
Are you still interested in adapting and finishing this PR or should I take over?
@@ -115,7 +115,7 @@ func init() { | |||
f.VarP(&forgetOptions.WithinWeekly, "keep-within-weekly", "", "keep weekly snapshots that are newer than `duration` (eg. 1y5m7d2h) relative to the latest snapshot") | |||
f.VarP(&forgetOptions.WithinMonthly, "keep-within-monthly", "", "keep monthly snapshots that are newer than `duration` (eg. 1y5m7d2h) relative to the latest snapshot") | |||
f.VarP(&forgetOptions.WithinYearly, "keep-within-yearly", "", "keep yearly snapshots that are newer than `duration` (eg. 1y5m7d2h) relative to the latest snapshot") | |||
f.Var(&forgetOptions.KeepTags, "keep-tag", "keep snapshots with this `taglist` (can be specified multiple times)") | |||
f.Var(&forgetOptions.KeepTags, "keep-tag", "keep snapshots with this `taglist` (can be specified multiple times, `taglist`s will be combined into one). this is a \"has all\" condition, i.e. a snapshot is only kept when it has all tags in `taglist`.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A snapshot must match all tags within a single taglist (AND), but the taglists are combined using OR.
Oh, that sounds like a more complicated fix. I'd be happy if you take over, I'm afraid I don't have the expertise and time, currently. |
Michael I also like that option, and as I can recall from the forum, some would indeed appreciate that delete-all option. I think its fine with the '--unsafe-allow-remove-all', though adding the extra requirement of path/host/tag also sounds good. I also won't have time at the moment and have yet evolved my go-mastery. |
I've opened #4764 as a replacement for this PR. Different from my previous suggestion that PR currently always forbids the complete deletion of a whole snapshot group if any |
What does this PR change? What problem does it solve?
This command would drop all snapshots. This is very dangerous and probably unwanted.
A suggested fix (see forum) is to only allow
--keep-tag
together with other--keep-*
options.While working on this, I noticed that another if case seem to have no effect, see comment in diff.
Was the change previously discussed in an issue or on the forum?
See also https://forum.restic.net/t/delete-all-snapshots-in-one-command-is-this-feature-intentional/6923/3
Fixes #4569
Checklist
changelog/unreleased/
that describes the changes for our users (see template).gofmt
on the code in all commits.