From 7926401cbd5cceaacd9509f2e50e1f7d636c2eb8 Mon Sep 17 00:00:00 2001 From: Krishnan Parthasarathi Date: Tue, 30 Apr 2024 18:11:10 -0700 Subject: [PATCH] ilm: Handle DeleteAllVersions action differently for DEL markers (#19481) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit i.e., this rule element doesn't apply to DEL markers. This is a breaking change to how ExpiredObejctDeleteAllVersions functions today. This is necessary to avoid the following highly probable footgun scenario in the future. Scenario: The user uses tags-based filtering to select an object's time to live(TTL). The application sometimes deletes objects, too, making its latest version a DEL marker. The previous implementation skipped tag-based filters if the newest version was DEL marker, voiding the tag-based TTL. The user is surprised to find objects that have expired sooner than expected. * Add DelMarkerExpiration action This ILM action removes all versions of an object if its the latest version is a DEL marker. ```xml 10 ``` 1. Applies only to objects whose, • The latest version is a DEL marker. • satisfies the number of days criteria 2. Deletes all versions of this object 3. Associated rule can't have tag-based filtering Includes, - New bucket event type for deletion due to DelMarkerExpiration --- cmd/data-scanner.go | 13 +- cmd/erasure-object.go | 9 +- internal/bucket/lifecycle/action_string.go | 7 +- .../bucket/lifecycle/delmarker-expiration.go | 74 +++++ .../lifecycle/delmarker-expiration_test.go | 63 +++++ internal/bucket/lifecycle/lifecycle.go | 82 +++--- internal/bucket/lifecycle/lifecycle_test.go | 252 +++++++++++++++--- internal/bucket/lifecycle/rule.go | 27 +- internal/bucket/lifecycle/rule_test.go | 25 ++ internal/event/name.go | 5 + internal/event/name_test.go | 3 + 11 files changed, 471 insertions(+), 89 deletions(-) create mode 100644 internal/bucket/lifecycle/delmarker-expiration.go create mode 100644 internal/bucket/lifecycle/delmarker-expiration_test.go diff --git a/cmd/data-scanner.go b/cmd/data-scanner.go index c5f9f3f8b..092db88b9 100644 --- a/cmd/data-scanner.go +++ b/cmd/data-scanner.go @@ -993,7 +993,7 @@ func (i *scannerItem) applyLifecycle(ctx context.Context, o ObjectLayer, oi Obje // This can happen when, // - ExpireObjectAllVersions flag is enabled // - NoncurrentVersionExpiration is applicable - case lifecycle.DeleteVersionAction, lifecycle.DeleteAllVersionsAction: + case lifecycle.DeleteVersionAction, lifecycle.DeleteAllVersionsAction, lifecycle.DelMarkerDeleteAllVersionsAction: size = 0 case lifecycle.DeleteAction: // On a non-versioned bucket, DeleteObject removes the only version permanently. @@ -1162,7 +1162,7 @@ func (i *scannerItem) applyActions(ctx context.Context, o ObjectLayer, oi Object // Note: objDeleted is true if and only if action == // lifecycle.DeleteAllVersionsAction - if action == lifecycle.DeleteAllVersionsAction { + if action.DeleteAll() { return true, 0 } @@ -1292,7 +1292,7 @@ func applyExpiryOnNonTransitionedObjects(ctx context.Context, objLayer ObjectLay if lcEvent.Action != lifecycle.NoneAction { numVersions := uint64(1) - if lcEvent.Action == lifecycle.DeleteAllVersionsAction { + if lcEvent.Action.DeleteAll() { numVersions = uint64(obj.NumVersions) } globalScannerMetrics.timeILM(lcEvent.Action)(numVersions) @@ -1320,8 +1320,11 @@ func applyExpiryOnNonTransitionedObjects(ctx context.Context, objLayer ObjectLay if obj.DeleteMarker { eventName = event.ObjectRemovedDeleteMarkerCreated } - if lcEvent.Action.DeleteAll() { + switch lcEvent.Action { + case lifecycle.DeleteAllVersionsAction: eventName = event.ObjectRemovedDeleteAllVersions + case lifecycle.DelMarkerDeleteAllVersionsAction: + eventName = event.ILMDelMarkerExpirationDelete } // Notify object deleted event. sendEvent(eventArgs{ @@ -1346,7 +1349,7 @@ func applyLifecycleAction(event lifecycle.Event, src lcEventSrc, obj ObjectInfo) switch action := event.Action; action { case lifecycle.DeleteVersionAction, lifecycle.DeleteAction, lifecycle.DeleteRestoredAction, lifecycle.DeleteRestoredVersionAction, - lifecycle.DeleteAllVersionsAction: + lifecycle.DeleteAllVersionsAction, lifecycle.DelMarkerDeleteAllVersionsAction: success = applyExpiryRule(event, src, obj) case lifecycle.TransitionAction, lifecycle.TransitionVersionAction: success = applyTransitionRule(event, src, obj) diff --git a/cmd/erasure-object.go b/cmd/erasure-object.go index 1195c03d5..b819d5ad3 100644 --- a/cmd/erasure-object.go +++ b/cmd/erasure-object.go @@ -1886,12 +1886,13 @@ func (er erasureObjects) DeleteObject(ctx context.Context, bucket, object string // based on the latest objectInfo and see if the object still // qualifies for deletion. if gerr == nil { - evt := evalActionFromLifecycle(ctx, *lc, rcfg, replcfg, goi) var isErr bool + evt := evalActionFromLifecycle(ctx, *lc, rcfg, replcfg, goi) switch evt.Action { - case lifecycle.NoneAction: - isErr = true - case lifecycle.TransitionAction, lifecycle.TransitionVersionAction: + case lifecycle.DeleteAllVersionsAction, lifecycle.DelMarkerDeleteAllVersionsAction: + // opts.DeletePrefix is used only in the above lifecycle Expiration actions. + default: + // object has been modified since lifecycle action was previously evaluated isErr = true } if isErr { diff --git a/internal/bucket/lifecycle/action_string.go b/internal/bucket/lifecycle/action_string.go index 0a7a55eed..e3f11ee59 100644 --- a/internal/bucket/lifecycle/action_string.go +++ b/internal/bucket/lifecycle/action_string.go @@ -16,12 +16,13 @@ func _() { _ = x[DeleteRestoredAction-5] _ = x[DeleteRestoredVersionAction-6] _ = x[DeleteAllVersionsAction-7] - _ = x[ActionCount-8] + _ = x[DelMarkerDeleteAllVersionsAction-8] + _ = x[ActionCount-9] } -const _Action_name = "NoneActionDeleteActionDeleteVersionActionTransitionActionTransitionVersionActionDeleteRestoredActionDeleteRestoredVersionActionDeleteAllVersionsActionActionCount" +const _Action_name = "NoneActionDeleteActionDeleteVersionActionTransitionActionTransitionVersionActionDeleteRestoredActionDeleteRestoredVersionActionDeleteAllVersionsActionDelMarkerDeleteAllVersionsActionActionCount" -var _Action_index = [...]uint8{0, 10, 22, 41, 57, 80, 100, 127, 150, 161} +var _Action_index = [...]uint8{0, 10, 22, 41, 57, 80, 100, 127, 150, 182, 193} func (i Action) String() string { if i < 0 || i >= Action(len(_Action_index)-1) { diff --git a/internal/bucket/lifecycle/delmarker-expiration.go b/internal/bucket/lifecycle/delmarker-expiration.go new file mode 100644 index 000000000..db22d2917 --- /dev/null +++ b/internal/bucket/lifecycle/delmarker-expiration.go @@ -0,0 +1,74 @@ +// Copyright (c) 2024 MinIO, Inc. +// +// This file is part of MinIO Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package lifecycle + +import ( + "encoding/xml" + "time" +) + +var errInvalidDaysDelMarkerExpiration = Errorf("Days must be a positive integer with DelMarkerExpiration") + +// DelMarkerExpiration used to xml encode/decode ILM action by the same name +type DelMarkerExpiration struct { + XMLName xml.Name `xml:"DelMarkerExpiration"` + Days int `xml:"Days,omitempty"` +} + +// Empty returns if a DelMarkerExpiration XML element is empty. +// Used to detect if lifecycle.Rule contained a DelMarkerExpiration element. +func (de DelMarkerExpiration) Empty() bool { + return de.Days == 0 +} + +// UnmarshalXML decodes a single XML element into a DelMarkerExpiration value +func (de *DelMarkerExpiration) UnmarshalXML(dec *xml.Decoder, start xml.StartElement) error { + type delMarkerExpiration DelMarkerExpiration + var dexp delMarkerExpiration + err := dec.DecodeElement(&dexp, &start) + if err != nil { + return err + } + + if dexp.Days <= 0 { + return errInvalidDaysDelMarkerExpiration + } + + *de = DelMarkerExpiration(dexp) + return nil +} + +// MarshalXML encodes a DelMarkerExpiration value into an XML element +func (de DelMarkerExpiration) MarshalXML(enc *xml.Encoder, start xml.StartElement) error { + if de.Empty() { + return nil + } + + type delMarkerExpiration DelMarkerExpiration + return enc.EncodeElement(delMarkerExpiration(de), start) +} + +// NextDue returns upcoming DelMarkerExpiration date for obj if +// applicable, returns false otherwise. +func (de DelMarkerExpiration) NextDue(obj ObjectOpts) (time.Time, bool) { + if !obj.IsLatest || !obj.DeleteMarker { + return time.Time{}, false + } + + return ExpectedExpiryTime(obj.ModTime, de.Days), true +} diff --git a/internal/bucket/lifecycle/delmarker-expiration_test.go b/internal/bucket/lifecycle/delmarker-expiration_test.go new file mode 100644 index 000000000..8cba948c7 --- /dev/null +++ b/internal/bucket/lifecycle/delmarker-expiration_test.go @@ -0,0 +1,63 @@ +// Copyright (c) 2024 MinIO, Inc. +// +// This file is part of MinIO Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package lifecycle + +import ( + "encoding/xml" + "fmt" + "testing" +) + +func TestDelMarkerExpParseAndValidate(t *testing.T) { + tests := []struct { + xml string + err error + }{ + { + xml: ` 1 `, + err: nil, + }, + { + xml: ` -1 `, + err: errInvalidDaysDelMarkerExpiration, + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("TestDelMarker-%d", i), func(t *testing.T) { + var dexp DelMarkerExpiration + var fail bool + err := xml.Unmarshal([]byte(test.xml), &dexp) + if test.err == nil { + if err != nil { + fail = true + } + } else { + if err == nil { + fail = true + } + if test.err.Error() != err.Error() { + fail = true + } + } + if fail { + t.Fatalf("Expected %v but got %v", test.err, err) + } + }) + } +} diff --git a/internal/bucket/lifecycle/lifecycle.go b/internal/bucket/lifecycle/lifecycle.go index 788027a77..d6ba7ae11 100644 --- a/internal/bucket/lifecycle/lifecycle.go +++ b/internal/bucket/lifecycle/lifecycle.go @@ -1,4 +1,4 @@ -// Copyright (c) 2015-2021 MinIO, Inc. +// Copyright (c) 2015-2024 MinIO, Inc. // // This file is part of MinIO Object Storage stack // @@ -22,7 +22,7 @@ import ( "fmt" "io" "net/http" - "sort" + "slices" "strings" "time" @@ -67,7 +67,8 @@ const ( DeleteRestoredVersionAction // DeleteAllVersionsAction deletes all versions when an object expires DeleteAllVersionsAction - + // DelMarkerDeleteAllVersionsAction deletes all versions when an object with delete marker as latest version expires + DelMarkerDeleteAllVersionsAction // ActionCount must be the last action and shouldn't be used as a regular action. ActionCount ) @@ -84,7 +85,7 @@ func (a Action) DeleteVersioned() bool { // DeleteAll - Returns true if the action demands deleting all versions of an object func (a Action) DeleteAll() bool { - return a == DeleteAllVersionsAction + return a == DeleteAllVersionsAction || a == DelMarkerDeleteAllVersionsAction } // Delete - Returns true if action demands delete on all objects (including restored) @@ -92,7 +93,7 @@ func (a Action) Delete() bool { if a.DeleteRestored() { return true } - return a == DeleteVersionAction || a == DeleteAction || a == DeleteAllVersionsAction + return a == DeleteVersionAction || a == DeleteAction || a == DeleteAllVersionsAction || a == DelMarkerDeleteAllVersionsAction } // Lifecycle - Configuration for bucket lifecycle. @@ -279,7 +280,7 @@ func (lc Lifecycle) FilterRules(obj ObjectOpts) []Rule { if !strings.HasPrefix(obj.Name, rule.GetPrefix()) { continue } - if !obj.DeleteMarker && !rule.Filter.TestTags(obj.UserTags) { + if !rule.Filter.TestTags(obj.UserTags) { continue } if !obj.DeleteMarker && !rule.Filter.BySize(obj.Size) { @@ -353,23 +354,6 @@ func (lc Lifecycle) eval(obj ObjectOpts, now time.Time) Event { } for _, rule := range lc.FilterRules(obj) { - if obj.IsLatest && rule.Expiration.DeleteAll.val { - if !rule.Expiration.IsDaysNull() { - // Specifying the Days tag will automatically perform all versions cleanup - // once the latest object is old enough to satisfy the age criteria. - // This is a MinIO only extension. - if expectedExpiry := ExpectedExpiryTime(obj.ModTime, int(rule.Expiration.Days)); now.IsZero() || now.After(expectedExpiry) { - events = append(events, Event{ - Action: DeleteAllVersionsAction, - RuleID: rule.ID, - Due: expectedExpiry, - }) - // No other conflicting actions apply to an all version expired object. - break - } - } - } - if obj.ExpiredObjectDeleteMarker() { if rule.Expiration.DeleteMarker.val { // Indicates whether MinIO will remove a delete marker with no noncurrent versions. @@ -401,6 +385,21 @@ func (lc Lifecycle) eval(obj ObjectOpts, now time.Time) Event { } } + // DelMarkerExpiration + if obj.IsLatest && obj.DeleteMarker && !rule.DelMarkerExpiration.Empty() { + if due, ok := rule.DelMarkerExpiration.NextDue(obj); ok && (now.IsZero() || now.After(due)) { + events = append(events, Event{ + Action: DelMarkerDeleteAllVersionsAction, + RuleID: rule.ID, + Due: due, + }) + } + // No other conflicting actions in this rule can apply to an object with current version as DEL marker + // Note: There could be other rules with earlier expiration which need to be considered. + // See TestDelMarkerExpiration + continue + } + // Skip rules with newer noncurrent versions specified. These rules are // not handled at an individual version level. eval applies only to a // specific version. @@ -448,11 +447,17 @@ func (lc Lifecycle) eval(obj ObjectOpts, now time.Time) Event { } case !rule.Expiration.IsDaysNull(): if expectedExpiry := ExpectedExpiryTime(obj.ModTime, int(rule.Expiration.Days)); now.IsZero() || now.After(expectedExpiry) { - events = append(events, Event{ + event := Event{ Action: DeleteAction, RuleID: rule.ID, Due: expectedExpiry, - }) + } + if rule.Expiration.DeleteAll.val { + // Expires all versions of this object once the latest object is old enough. + // This is a MinIO only extension. + event.Action = DeleteAllVersionsAction + } + events = append(events, event) } } @@ -470,25 +475,30 @@ func (lc Lifecycle) eval(obj ObjectOpts, now time.Time) Event { } if len(events) > 0 { - sort.Slice(events, func(i, j int) bool { + slices.SortFunc(events, func(a, b Event) int { // Prefer Expiration over Transition for both current // and noncurrent versions when, // - now is past the expected time to action // - expected time to action is the same for both actions - if now.After(events[i].Due) && now.After(events[j].Due) || events[i].Due.Equal(events[j].Due) { - switch events[i].Action { - case DeleteAction, DeleteVersionAction: - return true + if now.After(a.Due) && now.After(b.Due) || a.Due.Equal(b.Due) { + switch a.Action { + case DeleteAllVersionsAction, DelMarkerDeleteAllVersionsAction, + DeleteAction, DeleteVersionAction: + return -1 } - switch events[j].Action { - case DeleteAction, DeleteVersionAction: - return false + switch b.Action { + case DeleteAllVersionsAction, DelMarkerDeleteAllVersionsAction, + DeleteAction, DeleteVersionAction: + return 1 } - return true + return -1 } // Prefer earlier occurring event - return events[i].Due.Before(events[j].Due) + if a.Due.Before(b.Due) { + return -1 + } + return 1 }) return events[0] } @@ -517,7 +527,7 @@ func ExpectedExpiryTime(modTime time.Time, days int) time.Time { func (lc Lifecycle) SetPredictionHeaders(w http.ResponseWriter, obj ObjectOpts) { event := lc.eval(obj, time.Time{}) switch event.Action { - case DeleteAction, DeleteVersionAction, DeleteAllVersionsAction: + case DeleteAction, DeleteVersionAction, DeleteAllVersionsAction, DelMarkerDeleteAllVersionsAction: w.Header()[xhttp.AmzExpiration] = []string{ fmt.Sprintf(`expiry-date="%s", rule-id="%s"`, event.Due.Format(http.TimeFormat), event.RuleID), } diff --git a/internal/bucket/lifecycle/lifecycle_test.go b/internal/bucket/lifecycle/lifecycle_test.go index 0e8aaa79a..258d77ed2 100644 --- a/internal/bucket/lifecycle/lifecycle_test.go +++ b/internal/bucket/lifecycle/lifecycle_test.go @@ -115,10 +115,22 @@ func TestParseAndValidateLifecycleConfig(t *testing.T) { }, // Lifecycle with max noncurrent versions { - inputConfig: `rule>Enabled5`, + inputConfig: `ruleEnabled5`, expectedParsingErr: nil, expectedValidationErr: nil, }, + // Lifecycle with delmarker expiration + { + inputConfig: `ruleEnabled5`, + expectedParsingErr: nil, + expectedValidationErr: nil, + }, + // Lifecycle with empty delmarker expiration + { + inputConfig: `ruleEnabled`, + expectedParsingErr: errInvalidDaysDelMarkerExpiration, + expectedValidationErr: nil, + }, } for i, tc := range testCases { @@ -228,7 +240,8 @@ func TestEval(t *testing.T) { objectName string objectTags string objectModTime time.Time - isExpiredDelMarker bool + isDelMarker bool + hasManyVersions bool expectedAction Action isNoncurrent bool objectSuccessorModTime time.Time @@ -383,36 +396,52 @@ func TestEval(t *testing.T) { }, // Should delete expired delete marker right away { - inputConfig: `trueEnabled`, - objectName: "foodir/fooobject", - objectModTime: time.Now().UTC().Add(-1 * time.Hour), // Created one hour ago - isExpiredDelMarker: true, - expectedAction: DeleteVersionAction, + inputConfig: `trueEnabled`, + objectName: "foodir/fooobject", + objectModTime: time.Now().UTC().Add(-1 * time.Hour), // Created one hour ago + isDelMarker: true, + expectedAction: DeleteVersionAction, }, - // Should delete expired object right away with 1 day expiration + // Should not expire a delete marker; ExpiredObjectDeleteAllVersions applies only when current version is not a DEL marker. { - inputConfig: `1trueEnabled`, - objectName: "foodir/fooobject", - objectModTime: time.Now().UTC().Add(-10 * 24 * time.Hour), // Created 10 days ago - isExpiredDelMarker: true, - expectedAction: DeleteAllVersionsAction, + inputConfig: `1trueEnabled`, + objectName: "foodir/fooobject", + objectModTime: time.Now().UTC().Add(-10 * 24 * time.Hour), // Created 10 days ago + isDelMarker: true, + hasManyVersions: true, + expectedAction: NoneAction, + }, + // Should delete all versions of this object since the latest version has past the expiry days criteria + { + inputConfig: `1trueEnabled`, + objectName: "foodir/fooobject", + objectModTime: time.Now().UTC().Add(-10 * 24 * time.Hour), // Created 10 days ago + hasManyVersions: true, + expectedAction: DeleteAllVersionsAction, + }, + // TransitionAction applies since object doesn't meet the age criteria for DeleteAllVersions + { + inputConfig: `30true10WARM-1Enabled`, + objectName: "foodir/fooobject", + objectModTime: time.Now().UTC().Add(-11 * 24 * time.Hour), // Created 11 days ago + hasManyVersions: true, + expectedAction: TransitionAction, }, - // Should not delete expired marker if its time has not come yet { - inputConfig: `Enabled1`, - objectName: "foodir/fooobject", - objectModTime: time.Now().UTC().Add(-12 * time.Hour), // Created 12 hours ago - isExpiredDelMarker: true, - expectedAction: NoneAction, + inputConfig: `Enabled1`, + objectName: "foodir/fooobject", + objectModTime: time.Now().UTC().Add(-12 * time.Hour), // Created 12 hours ago + isDelMarker: true, + expectedAction: NoneAction, }, // Should delete expired marker since its time has come { - inputConfig: `Enabled1`, - objectName: "foodir/fooobject", - objectModTime: time.Now().UTC().Add(-10 * 24 * time.Hour), // Created 10 days ago - isExpiredDelMarker: true, - expectedAction: DeleteVersionAction, + inputConfig: `Enabled1`, + objectName: "foodir/fooobject", + objectModTime: time.Now().UTC().Add(-10 * 24 * time.Hour), // Created 10 days ago + isDelMarker: true, + expectedAction: DeleteVersionAction, }, // Should transition immediately when Transition days is zero { @@ -579,6 +608,82 @@ func TestEval(t *testing.T) { objectSuccessorModTime: time.Now().UTC().Add(-90 * 24 * time.Hour), expectedAction: DeleteVersionAction, }, + { + // DelMarkerExpiration is preferred since object age is past both transition and expiration days. + inputConfig: ` + + DelMarkerExpiration with Transition + + Enabled + + 60 + + + WARM-1 + 30 + + + `, + objectName: "obj-1", + objectModTime: time.Now().UTC().Add(-90 * 24 * time.Hour), + isDelMarker: true, + expectedAction: DelMarkerDeleteAllVersionsAction, + }, + { + // NoneAction since object doesn't qualify for DelMarkerExpiration yet. + // Note: TransitionAction doesn't apply to DEL marker + inputConfig: ` + + DelMarkerExpiration with Transition + + Enabled + + 60 + + + WARM-1 + 30 + + + `, + objectName: "obj-1", + objectModTime: time.Now().UTC().Add(-50 * 24 * time.Hour), + isDelMarker: true, + expectedAction: NoneAction, + }, + { + inputConfig: ` + + DelMarkerExpiration with non DEL-marker object + + Enabled + + 60 + + + `, + objectName: "obj-1", + objectModTime: time.Now().UTC().Add(-90 * 24 * time.Hour), + expectedAction: NoneAction, + }, + { + inputConfig: ` + + DelMarkerExpiration with noncurrent DEL-marker + + Enabled + + 60 + + + `, + objectName: "obj-1", + objectModTime: time.Now().UTC().Add(-90 * 24 * time.Hour), + objectSuccessorModTime: time.Now().UTC().Add(-60 * 24 * time.Hour), + isDelMarker: true, + isNoncurrent: true, + expectedAction: NoneAction, + }, } for _, tc := range testCases { @@ -588,16 +693,20 @@ func TestEval(t *testing.T) { if err != nil { t.Fatalf("Got unexpected error: %v", err) } - if res := lc.Eval(ObjectOpts{ + opts := ObjectOpts{ Name: tc.objectName, UserTags: tc.objectTags, ModTime: tc.objectModTime, - DeleteMarker: tc.isExpiredDelMarker, - NumVersions: 1, + DeleteMarker: tc.isDelMarker, IsLatest: !tc.isNoncurrent, SuccessorModTime: tc.objectSuccessorModTime, VersionID: tc.versionID, - }); res.Action != tc.expectedAction { + } + opts.NumVersions = 1 + if tc.hasManyVersions { + opts.NumVersions = 2 // at least one noncurrent version + } + if res := lc.Eval(opts); res.Action != tc.expectedAction { t.Fatalf("Expected action: `%v`, got: `%v`", tc.expectedAction, res.Action) } }) @@ -1160,7 +1269,7 @@ func TestFilterRules(t *testing.T) { opts ObjectOpts hasRules bool }{ - { // Delete marker should match filter without tags + { // Delete marker shouldn't match filter without tags lc: Lifecycle{ Rules: []Rule{ rules[0], @@ -1171,7 +1280,7 @@ func TestFilterRules(t *testing.T) { IsLatest: true, Name: "obj-1", }, - hasRules: true, + hasRules: false, }, { // PUT version with no matching tags lc: Lifecycle{ @@ -1269,3 +1378,86 @@ func TestFilterRules(t *testing.T) { }) } } + +// TestDeleteAllVersions tests ordering among events, especially ones which +// expire all versions like ExpiredObjectDeleteAllVersions and +// DelMarkerExpiration +func TestDeleteAllVersions(t *testing.T) { + // ExpiredObjectDeleteAllVersions + lc := Lifecycle{ + Rules: []Rule{ + { + ID: "ExpiredObjectDeleteAllVersions-20", + Status: "Enabled", + Expiration: Expiration{ + set: true, + DeleteAll: Boolean{val: true, set: true}, + Days: 20, + }, + }, + { + ID: "Transition-10", + Status: "Enabled", + Transition: Transition{ + set: true, + StorageClass: "WARM-1", + Days: 10, + }, + }, + }, + } + opts := ObjectOpts{ + Name: "foo.txt", + ModTime: time.Now().UTC().Add(-10 * 24 * time.Hour), // created 10 days ago + Size: 0, + VersionID: uuid.New().String(), + IsLatest: true, + NumVersions: 4, + } + + event := lc.eval(opts, time.Time{}) + if event.Action != TransitionAction { + t.Fatalf("Expected %v action but got %v", TransitionAction, event.Action) + } + // The earlier upcoming lifecycle event must be picked, i.e rule with id "Transition-10" + if exp := ExpectedExpiryTime(opts.ModTime, 10); exp != event.Due { + t.Fatalf("Expected due %v but got %v, ruleID=%v", exp, event.Due, event.RuleID) + } + + // DelMarkerExpiration + lc = Lifecycle{ + Rules: []Rule{ + { + ID: "delmarker-exp-20", + Status: "Enabled", + DelMarkerExpiration: DelMarkerExpiration{ + Days: 20, + }, + }, + { + ID: "delmarker-exp-10", + Status: "Enabled", + DelMarkerExpiration: DelMarkerExpiration{ + Days: 10, + }, + }, + }, + } + opts = ObjectOpts{ + Name: "foo.txt", + ModTime: time.Now().UTC().Add(-10 * 24 * time.Hour), // created 10 days ago + Size: 0, + VersionID: uuid.New().String(), + IsLatest: true, + DeleteMarker: true, + NumVersions: 4, + } + event = lc.eval(opts, time.Time{}) + if event.Action != DelMarkerDeleteAllVersionsAction { + t.Fatalf("Expected %v action but got %v", DelMarkerDeleteAllVersionsAction, event.Action) + } + // The earlier upcoming lifecycle event must be picked, i.e rule with id "delmarker-exp-10" + if exp := ExpectedExpiryTime(opts.ModTime, 10); exp != event.Due { + t.Fatalf("Expected due %v but got %v, ruleID=%v", exp, event.Due, event.RuleID) + } +} diff --git a/internal/bucket/lifecycle/rule.go b/internal/bucket/lifecycle/rule.go index 147b0d6eb..67e026b1b 100644 --- a/internal/bucket/lifecycle/rule.go +++ b/internal/bucket/lifecycle/rule.go @@ -33,22 +33,24 @@ const ( // Rule - a rule for lifecycle configuration. type Rule struct { - XMLName xml.Name `xml:"Rule"` - ID string `xml:"ID,omitempty"` - Status Status `xml:"Status"` - Filter Filter `xml:"Filter,omitempty"` - Prefix Prefix `xml:"Prefix,omitempty"` - Expiration Expiration `xml:"Expiration,omitempty"` - Transition Transition `xml:"Transition,omitempty"` + XMLName xml.Name `xml:"Rule"` + ID string `xml:"ID,omitempty"` + Status Status `xml:"Status"` + Filter Filter `xml:"Filter,omitempty"` + Prefix Prefix `xml:"Prefix,omitempty"` + Expiration Expiration `xml:"Expiration,omitempty"` + Transition Transition `xml:"Transition,omitempty"` + DelMarkerExpiration DelMarkerExpiration `xml:"DelMarkerExpiration,omitempty"` // FIXME: add a type to catch unsupported AbortIncompleteMultipartUpload AbortIncompleteMultipartUpload `xml:"AbortIncompleteMultipartUpload,omitempty"` NoncurrentVersionExpiration NoncurrentVersionExpiration `xml:"NoncurrentVersionExpiration,omitempty"` NoncurrentVersionTransition NoncurrentVersionTransition `xml:"NoncurrentVersionTransition,omitempty"` } var ( - errInvalidRuleID = Errorf("ID length is limited to 255 characters") - errEmptyRuleStatus = Errorf("Status should not be empty") - errInvalidRuleStatus = Errorf("Status must be set to either Enabled or Disabled") + errInvalidRuleID = Errorf("ID length is limited to 255 characters") + errEmptyRuleStatus = Errorf("Status should not be empty") + errInvalidRuleStatus = Errorf("Status must be set to either Enabled or Disabled") + errInvalidRuleDelMarkerExpiration = Errorf("Rule with DelMarkerExpiration cannot have tags based filtering") ) // validateID - checks if ID is valid or not. @@ -158,7 +160,10 @@ func (r Rule) Validate() error { if err := r.validateNoncurrentTransition(); err != nil { return err } - if !r.Expiration.set && !r.Transition.set && !r.NoncurrentVersionExpiration.set && !r.NoncurrentVersionTransition.set { + if (!r.Filter.Tag.IsEmpty() || len(r.Filter.And.Tags) != 0) && !r.DelMarkerExpiration.Empty() { + return errInvalidRuleDelMarkerExpiration + } + if !r.Expiration.set && !r.Transition.set && !r.NoncurrentVersionExpiration.set && !r.NoncurrentVersionTransition.set && r.DelMarkerExpiration.Empty() { return errXMLNotWellFormed } return nil diff --git a/internal/bucket/lifecycle/rule_test.go b/internal/bucket/lifecycle/rule_test.go index 94b9bccd3..f6f139174 100644 --- a/internal/bucket/lifecycle/rule_test.go +++ b/internal/bucket/lifecycle/rule_test.go @@ -105,6 +105,31 @@ func TestInvalidRules(t *testing.T) { `, expectedErr: errXMLNotWellFormed, }, + { + inputXML: ` + Rule with a tag and DelMarkerExpiration + k1v1 + + 365 + + Enabled + `, + expectedErr: errInvalidRuleDelMarkerExpiration, + }, + { + inputXML: ` + Rule with multiple tags and DelMarkerExpiration + + k1v1 + k2v2 + + + 365 + + Enabled + `, + expectedErr: errInvalidRuleDelMarkerExpiration, + }, } for i, tc := range invalidTestCases { diff --git a/internal/event/name.go b/internal/event/name.go index 1e52621ce..184c10383 100644 --- a/internal/event/name.go +++ b/internal/event/name.go @@ -63,6 +63,7 @@ const ( ObjectManyVersions ObjectLargeVersions PrefixManyFolders + ILMDelMarkerExpirationDelete objectSingleTypesEnd // Start Compound types that require expansion: @@ -199,6 +200,8 @@ func (name Name) String() string { return "s3:ObjectRemoved:NoOP" case ObjectRemovedDeleteAllVersions: return "s3:ObjectRemoved:DeleteAllVersions" + case ILMDelMarkerExpirationDelete: + return "s3:LifecycleDelMarkerExpiration:Delete" case ObjectReplicationAll: return "s3:Replication:*" case ObjectReplicationFailed: @@ -324,6 +327,8 @@ func ParseName(s string) (Name, error) { return ObjectRemovedNoOP, nil case "s3:ObjectRemoved:DeleteAllVersions": return ObjectRemovedDeleteAllVersions, nil + case "s3:LifecycleDelMarkerExpiration:Delete": + return ILMDelMarkerExpirationDelete, nil case "s3:Replication:*": return ObjectReplicationAll, nil case "s3:Replication:OperationFailedReplication": diff --git a/internal/event/name_test.go b/internal/event/name_test.go index 81a32a4a8..7bafa2ee9 100644 --- a/internal/event/name_test.go +++ b/internal/event/name_test.go @@ -68,6 +68,8 @@ func TestNameString(t *testing.T) { {ObjectCreatedPut, "s3:ObjectCreated:Put"}, {ObjectRemovedAll, "s3:ObjectRemoved:*"}, {ObjectRemovedDelete, "s3:ObjectRemoved:Delete"}, + {ObjectRemovedDeleteAllVersions, "s3:ObjectRemoved:DeleteAllVersions"}, + {ILMDelMarkerExpirationDelete, "s3:LifecycleDelMarkerExpiration:Delete"}, {ObjectRemovedNoOP, "s3:ObjectRemoved:NoOP"}, {ObjectCreatedPutRetention, "s3:ObjectCreated:PutRetention"}, {ObjectCreatedPutLegalHold, "s3:ObjectCreated:PutLegalHold"}, @@ -219,6 +221,7 @@ func TestParseName(t *testing.T) { {"s3:ObjectAccessed:*", ObjectAccessedAll, false}, {"s3:ObjectRemoved:Delete", ObjectRemovedDelete, false}, {"s3:ObjectRemoved:NoOP", ObjectRemovedNoOP, false}, + {"s3:LifecycleDelMarkerExpiration:Delete", ILMDelMarkerExpirationDelete, false}, {"", blankName, true}, }