From 48aebf2d9df81899200348af28820bad23e7efdb Mon Sep 17 00:00:00 2001
From: findmyname666 <35902428+findmyname666@users.noreply.github.com>
Date: Thu, 16 Jul 2020 16:39:41 +0200
Subject: [PATCH] allow lifecycle rules with overlapping prefixes (#10053)

---
 pkg/bucket/lifecycle/lifecycle.go      | 14 ++++----
 pkg/bucket/lifecycle/lifecycle_test.go | 47 ++++++++++++++++++++++----
 2 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/pkg/bucket/lifecycle/lifecycle.go b/pkg/bucket/lifecycle/lifecycle.go
index 8b596ea60..2a53caa0a 100644
--- a/pkg/bucket/lifecycle/lifecycle.go
+++ b/pkg/bucket/lifecycle/lifecycle.go
@@ -24,9 +24,9 @@ import (
 )
 
 var (
-	errLifecycleTooManyRules      = Errorf("Lifecycle configuration allows a maximum of 1000 rules")
-	errLifecycleNoRule            = Errorf("Lifecycle configuration should have at least one rule")
-	errLifecycleOverlappingPrefix = Errorf("Lifecycle configuration has rules with overlapping prefix")
+	errLifecycleTooManyRules = Errorf("Lifecycle configuration allows a maximum of 1000 rules")
+	errLifecycleNoRule       = Errorf("Lifecycle configuration should have at least one rule")
+	errLifecycleDuplicateID  = Errorf("Lifecycle configuration has rule with the same ID. Rule ID must be unique.")
 )
 
 // Action represents a delete action or other transition
@@ -115,17 +115,15 @@ func (lc Lifecycle) Validate() error {
 			return err
 		}
 	}
-	// Compare every rule's prefix with every other rule's prefix
+	// Make sure Rule ID is unique
 	for i := range lc.Rules {
 		if i == len(lc.Rules)-1 {
 			break
 		}
-		// N B Empty prefixes overlap with all prefixes
 		otherRules := lc.Rules[i+1:]
 		for _, otherRule := range otherRules {
-			if strings.HasPrefix(lc.Rules[i].Prefix(), otherRule.Prefix()) ||
-				strings.HasPrefix(otherRule.Prefix(), lc.Rules[i].Prefix()) {
-				return errLifecycleOverlappingPrefix
+			if lc.Rules[i].ID == otherRule.ID {
+				return errLifecycleDuplicateID
 			}
 		}
 	}
diff --git a/pkg/bucket/lifecycle/lifecycle_test.go b/pkg/bucket/lifecycle/lifecycle_test.go
index 2c27b897c..10bc547fe 100644
--- a/pkg/bucket/lifecycle/lifecycle_test.go
+++ b/pkg/bucket/lifecycle/lifecycle_test.go
@@ -27,11 +27,12 @@ import (
 func TestParseAndValidateLifecycleConfig(t *testing.T) {
 	// Test for  lifecycle config with more than 1000 rules
 	var manyRules []Rule
-	rule := Rule{
-		Status:     "Enabled",
-		Expiration: Expiration{Days: ExpirationDays(3)},
-	}
 	for i := 0; i < 1001; i++ {
+		rule := Rule{
+			ID:         fmt.Sprintf("toManyRule%d", i),
+			Status:     "Enabled",
+			Expiration: Expiration{Days: ExpirationDays(i)},
+		}
 		manyRules = append(manyRules, rule)
 	}
 
@@ -42,6 +43,7 @@ func TestParseAndValidateLifecycleConfig(t *testing.T) {
 
 	// Test for lifecycle config with rules containing overlapping prefixes
 	rule1 := Rule{
+		ID:         "rule1",
 		Status:     "Enabled",
 		Expiration: Expiration{Days: ExpirationDays(3)},
 		Filter: Filter{
@@ -49,6 +51,7 @@ func TestParseAndValidateLifecycleConfig(t *testing.T) {
 		},
 	}
 	rule2 := Rule{
+		ID:         "rule2",
 		Status:     "Enabled",
 		Expiration: Expiration{Days: ExpirationDays(3)},
 		Filter: Filter{
@@ -63,6 +66,31 @@ func TestParseAndValidateLifecycleConfig(t *testing.T) {
 		t.Fatal("Failed to marshal lifecycle config with rules having overlapping prefix")
 	}
 
+	// Test for lifecycle rules with duplicate IDs
+	rule3 := Rule{
+		ID:         "duplicateID",
+		Status:     "Enabled",
+		Expiration: Expiration{Days: ExpirationDays(3)},
+		Filter: Filter{
+			Prefix: "/a/b",
+		},
+	}
+	rule4 := Rule{
+		ID:         "duplicateID",
+		Status:     "Enabled",
+		Expiration: Expiration{Days: ExpirationDays(4)},
+		Filter: Filter{
+			And: And{
+				Prefix: "/x/z",
+			},
+		},
+	}
+	duplicateIDRules := []Rule{rule3, rule4}
+	duplicateIDLcConfig, err := xml.Marshal(Lifecycle{Rules: duplicateIDRules})
+	if err != nil {
+		t.Fatal("Failed to marshal lifecycle config of rules with duplicate ID.")
+	}
+
 	testCases := []struct {
 		inputConfig           string
 		expectedParsingErr    error
@@ -70,7 +98,8 @@ func TestParseAndValidateLifecycleConfig(t *testing.T) {
 	}{
 		{ // Valid lifecycle config
 			inputConfig: `<LifecycleConfiguration>
-		                          <Rule>
+								  <Rule>
+								  <ID>testRule1</ID>
 		                          <Filter>
 		                             <Prefix>prefix</Prefix>
 		                          </Filter>
@@ -78,6 +107,7 @@ func TestParseAndValidateLifecycleConfig(t *testing.T) {
 		                          <Expiration><Days>3</Days></Expiration>
 		                          </Rule>
 		                              <Rule>
+								  <ID>testRule2</ID>
 		                          <Filter>
 		                             <Prefix>another-prefix</Prefix>
 		                          </Filter>
@@ -114,7 +144,12 @@ func TestParseAndValidateLifecycleConfig(t *testing.T) {
 		{ // lifecycle config with rules having overlapping prefix
 			inputConfig:           string(overlappingLcConfig),
 			expectedParsingErr:    nil,
-			expectedValidationErr: errLifecycleOverlappingPrefix,
+			expectedValidationErr: nil,
+		},
+		{ // lifecycle config with rules having overlapping prefix
+			inputConfig:           string(duplicateIDLcConfig),
+			expectedParsingErr:    nil,
+			expectedValidationErr: errLifecycleDuplicateID,
 		},
 	}