From 6c07bfee8ad3a667722a39440637d1ed48bad8b7 Mon Sep 17 00:00:00 2001
From: Krishnan Parthasarathi <krisis@users.noreply.github.com>
Date: Fri, 3 May 2024 04:18:58 -0700
Subject: [PATCH] With retention, skip actions expiring all versions (#19657)

ILM actions due to ExpiredObjectDeleteAllVersions and
DelMarkerExpiration are ignored when object locking is enabled on a
bucket.
Note: This applies to object versions which may not have retention
configured on them. This applies to all object versions in this bucket,
including those created before the retention config was applied.
---
 cmd/data-scanner.go      | 14 +++---
 cmd/data-scanner_test.go | 96 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+), 8 deletions(-)

diff --git a/cmd/data-scanner.go b/cmd/data-scanner.go
index 092db88b9..9aa01f655 100644
--- a/cmd/data-scanner.go
+++ b/cmd/data-scanner.go
@@ -1199,17 +1199,15 @@ func evalActionFromLifecycle(ctx context.Context, lc lifecycle.Lifecycle, lr loc
 		console.Debugf(applyActionsLogPrefix+" lifecycle: Secondary scan: %v\n", event.Action)
 	}
 
-	if event.Action == lifecycle.NoneAction {
-		return event
-	}
-
-	if obj.IsLatest && event.Action == lifecycle.DeleteAllVersionsAction {
-		if lr.LockEnabled && enforceRetentionForDeletion(ctx, obj) {
+	switch event.Action {
+	case lifecycle.DeleteAllVersionsAction, lifecycle.DelMarkerDeleteAllVersionsAction:
+		// Skip if bucket has object locking enabled; To prevent the
+		// possibility of violating an object retention on one of the
+		// noncurrent versions of this object.
+		if lr.LockEnabled {
 			return lifecycle.Event{Action: lifecycle.NoneAction}
 		}
-	}
 
-	switch event.Action {
 	case lifecycle.DeleteVersionAction, lifecycle.DeleteRestoredVersionAction:
 		// Defensive code, should never happen
 		if obj.VersionID == "" {
diff --git a/cmd/data-scanner_test.go b/cmd/data-scanner_test.go
index a55007a67..325131e49 100644
--- a/cmd/data-scanner_test.go
+++ b/cmd/data-scanner_test.go
@@ -20,12 +20,15 @@ package cmd
 import (
 	"context"
 	"encoding/xml"
+	"fmt"
+	"strings"
 	"sync"
 	"testing"
 	"time"
 
 	"github.com/google/uuid"
 	"github.com/minio/minio/internal/bucket/lifecycle"
+	"github.com/minio/minio/internal/bucket/object/lock"
 	"github.com/minio/minio/internal/bucket/versioning"
 )
 
@@ -141,3 +144,96 @@ func TestApplyNewerNoncurrentVersionsLimit(t *testing.T) {
 		}
 	}
 }
+
+func TestEvalActionFromLifecycle(t *testing.T) {
+	// Tests cover only ExpiredObjectDeleteAllVersions and DelMarkerExpiration actions
+	obj := ObjectInfo{
+		Name:        "foo",
+		ModTime:     time.Now().Add(-31 * 24 * time.Hour),
+		Size:        100 << 20,
+		VersionID:   uuid.New().String(),
+		IsLatest:    true,
+		NumVersions: 4,
+	}
+	delMarker := ObjectInfo{
+		Name:         "foo-deleted",
+		ModTime:      time.Now().Add(-61 * 24 * time.Hour),
+		Size:         0,
+		VersionID:    uuid.New().String(),
+		IsLatest:     true,
+		DeleteMarker: true,
+		NumVersions:  4,
+	}
+	deleteAllILM := `<LifecycleConfiguration>
+			    <Rule>
+		               <Expiration>
+		                  <Days>30</Days>
+	                          <ExpiredObjectAllVersions>true</ExpiredObjectAllVersions>
+	                       </Expiration>
+	                       <Filter></Filter>
+	                       <Status>Enabled</Status>
+			       <ID>DeleteAllVersions</ID>
+	                    </Rule>
+	                 </LifecycleConfiguration>`
+	delMarkerILM := `<LifecycleConfiguration>
+                            <Rule>
+                              <ID>DelMarkerExpiration</ID>
+                              <Filter></Filter>
+                              <Status>Enabled</Status>
+                              <DelMarkerExpiration>
+                                <Days>60</Days>
+                              </DelMarkerExpiration>
+                             </Rule>
+                       </LifecycleConfiguration>`
+	deleteAllLc, err := lifecycle.ParseLifecycleConfig(strings.NewReader(deleteAllILM))
+	if err != nil {
+		t.Fatalf("Failed to parse deleteAllILM test ILM policy %v", err)
+	}
+	delMarkerLc, err := lifecycle.ParseLifecycleConfig(strings.NewReader(delMarkerILM))
+	if err != nil {
+		t.Fatalf("Failed to parse delMarkerILM test ILM policy %v", err)
+	}
+	tests := []struct {
+		ilm       lifecycle.Lifecycle
+		retention lock.Retention
+		obj       ObjectInfo
+		want      lifecycle.Action
+	}{
+		{
+			// with object locking
+			ilm:       *deleteAllLc,
+			retention: lock.Retention{LockEnabled: true},
+			obj:       obj,
+			want:      lifecycle.NoneAction,
+		},
+		{
+			// without object locking
+			ilm:       *deleteAllLc,
+			retention: lock.Retention{},
+			obj:       obj,
+			want:      lifecycle.DeleteAllVersionsAction,
+		},
+		{
+			// with object locking
+			ilm:       *delMarkerLc,
+			retention: lock.Retention{LockEnabled: true},
+			obj:       delMarker,
+			want:      lifecycle.NoneAction,
+		},
+		{
+			// without object locking
+			ilm:       *delMarkerLc,
+			retention: lock.Retention{},
+			obj:       delMarker,
+			want:      lifecycle.DelMarkerDeleteAllVersionsAction,
+		},
+	}
+
+	for i, test := range tests {
+		t.Run(fmt.Sprintf("TestEvalAction-%d", i), func(t *testing.T) {
+			if got := evalActionFromLifecycle(context.TODO(), test.ilm, test.retention, nil, test.obj); got.Action != test.want {
+				t.Fatalf("Expected %v but got %v", test.want, got)
+			}
+		})
+	}
+}