Fix inconsistent metadata after healing (#14125)

When calculating signatures empty part ETags were not discarded, leading 
to a different signature compared to freshly created ones.

This would mean that after a heal signature of the healed metadata would be 
different. Fixing the calculation of signature will make these consistent.

Furthermore when inconsistent entries, with zero version ID, with the same 
mod times but different signatures, the one with the lowest signature would 
be picked for quorum check. Since this is 50/50, we fall back to a simple 
quorum count on all signatures.

Each of these fixes by themselves will lead to quorum. Tests were added 
for regressions and expected outcomes.

cmd/xl-storage-format-v2-legacy.go

@@ -83,7 +83,22 @@ func (j *xlMetaV2Version) unmarshalV(v uint8, bts []byte) (o []byte, err error)
 	switch v {
 	// We accept un-set as latest version.
 	case 0, xlMetaVersion:
-		return j.UnmarshalMsg(bts)
+		o, err = j.UnmarshalMsg(bts)
+
+		// Clean up PartEtags on v1
+		if j.ObjectV2 != nil {
+			allEmpty := true
+			for _, tag := range j.ObjectV2.PartETags {
+				if len(tag) != 0 {
+					allEmpty = false
+					break
+				}
+			}
+			if allEmpty {
+				j.ObjectV2.PartETags = nil
+			}
+		}
+		return o, err
 	}
 	return bts, fmt.Errorf("unknown xlMetaVersion: %d", v)
 }

cmd/xl-storage-format-v2.go

@@ -264,9 +264,13 @@ func (x xlMetaV2VersionHeader) String() string {
 
 // matchesNotStrict returns whether x and o have both have non-zero version,
 // their versions match and their type match.
+// If they have zero version, modtime must match.
 func (x xlMetaV2VersionHeader) matchesNotStrict(o xlMetaV2VersionHeader) bool {
-	return x.VersionID != [16]byte{} &&
+	if x.VersionID == [16]byte{} {
-		x.VersionID == o.VersionID &&
+		return x.VersionID == o.VersionID &&
+			x.Type == o.Type && o.ModTime == x.ModTime
+	}
+	return x.VersionID == o.VersionID &&
 		x.Type == o.Type
 }
 
@@ -508,7 +512,14 @@ func (j *xlMetaV2Object) Signature() [4]byte {
 	c.ErasureIndex = 0
 
 	// Nil 0 size allownil, so we don't differentiate between nil and 0 len.
-	if len(c.PartETags) == 0 {
+	allEmpty := true
+	for _, tag := range c.PartETags {
+		if len(tag) != 0 {
+			allEmpty = false
+			break
+		}
+	}
+	if allEmpty {
 		c.PartETags = nil
 	}
 	if len(c.PartActualSizes) == 0 {
@@ -1676,6 +1687,9 @@ func (x xlMetaV2) ListVersions(volume, path string) ([]FileInfo, error) {
 // Quorum should be > 1 and <= len(versions).
 // If strict is set to false, entries that match type
 func mergeXLV2Versions(quorum int, strict bool, versions ...[]xlMetaV2ShallowVersion) (merged []xlMetaV2ShallowVersion) {
+	if quorum <= 0 {
+		quorum = 1
+	}
 	if len(versions) < quorum || len(versions) == 0 {
 		return nil
 	}
@@ -1686,14 +1700,16 @@ func mergeXLV2Versions(quorum int, strict bool, versions ...[]xlMetaV2ShallowVer
 		// No need for non-strict checks if quorum is 1.
 		strict = true
 	}
+	// Shallow copy input
+	versions = append(make([][]xlMetaV2ShallowVersion, 0, len(versions)), versions...)
+
 	// Our result
 	merged = make([]xlMetaV2ShallowVersion, 0, len(versions[0]))
 	tops := make([]xlMetaV2ShallowVersion, len(versions))
 	for {
 		// Step 1 create slice with all top versions.
 		tops = tops[:0]
-		var topSig [4]byte
+		var topSig xlMetaV2VersionHeader
-		var topID [16]byte
 		consistent := true // Are all signatures consistent (shortcut)
 		for _, vers := range versions {
 			if len(vers) == 0 {
@@ -1703,10 +1719,9 @@ func mergeXLV2Versions(quorum int, strict bool, versions ...[]xlMetaV2ShallowVer
 			ver := vers[0]
 			if len(tops) == 0 {
 				consistent = true
-				topSig = ver.header.Signature
+				topSig = ver.header
-				topID = ver.header.VersionID
 			} else {
-				consistent = consistent && topSig == ver.header.Signature && topID == ver.header.VersionID
+				consistent = consistent && ver.header == topSig
 			}
 			tops = append(tops, vers[0])
 		}
@@ -1732,7 +1747,7 @@ func mergeXLV2Versions(quorum int, strict bool, versions ...[]xlMetaV2ShallowVer
 					continue
 				}
 				if i == 0 || ver.header.sortsBefore(latest.header) {
-					if i == 0 {
+					if i == 0 || latestCount == 0 {
 						latestCount = 1
 					} else if !strict && ver.header.matchesNotStrict(latest.header) {
 						latestCount++
@@ -1744,12 +1759,45 @@ func mergeXLV2Versions(quorum int, strict bool, versions ...[]xlMetaV2ShallowVer
 				}
 
 				// Mismatch, but older.
-				if !strict && ver.header.matchesNotStrict(latest.header) {
+				if latestCount > 0 && !strict && ver.header.matchesNotStrict(latest.header) {
-					// If non-nil version ID and it matches, assume match, but keep newest.
-					if ver.header.sortsBefore(latest.header) {
-						latest = ver
-					}
 					latestCount++
+					continue
+				}
+				if latestCount > 0 && ver.header.VersionID == latest.header.VersionID {
+					// Version IDs match, but otherwise unable to resolve.
+					// We are either strict, or don't have enough information to match.
+					// Switch to a pure counting algo.
+					x := make(map[xlMetaV2VersionHeader]int, len(tops))
+					for _, a := range tops {
+						if a.header.VersionID != ver.header.VersionID {
+							continue
+						}
+						if !strict {
+							a.header.Signature = [4]byte{}
+						}
+						x[a.header]++
+					}
+					latestCount = 0
+					for k, v := range x {
+						if v < latestCount {
+							continue
+						}
+						if v == latestCount && latest.header.sortsBefore(k) {
+							// Tiebreak, use sort.
+							continue
+						}
+						for _, a := range tops {
+							hdr := a.header
+							if !strict {
+								hdr.Signature = [4]byte{}
+							}
+							if hdr == k {
+								latest = a
+							}
+						}
+						latestCount = v
+					}
+					break
 				}
 			}
 			if latestCount >= quorum {

cmd/xl-storage-format-v2_test.go

@@ -19,11 +19,15 @@ package cmd
 
 import (
 	"bytes"
+	"encoding/json"
+	"fmt"
+	"io"
 	"sort"
 	"testing"
 	"time"
 
 	"github.com/google/uuid"
+	"github.com/klauspost/compress/zip"
 	"github.com/klauspost/compress/zstd"
 	"github.com/minio/minio/internal/bucket/lifecycle"
 	xhttp "github.com/minio/minio/internal/http"
@@ -476,3 +480,272 @@ func Test_xlMetaV2Shallow_Load(t *testing.T) {
 		test(t, &xl)
 	})
 }
+
+func Test_mergeXLV2Versions(t *testing.T) {
+	dataZ, err := ioutil.ReadFile("testdata/xl-meta-consist.zip")
+	if err != nil {
+		t.Fatal(err)
+	}
+	var vers [][]xlMetaV2ShallowVersion
+	zr, err := zip.NewReader(bytes.NewReader(dataZ), int64(len(dataZ)))
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, file := range zr.File {
+		if file.UncompressedSize64 == 0 {
+			continue
+		}
+		in, err := file.Open()
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer in.Close()
+		buf, err := io.ReadAll(in)
+		if err != nil {
+			t.Fatal(err)
+		}
+		var xl xlMetaV2
+		err = xl.LoadOrConvert(buf)
+		if err != nil {
+			t.Fatal(err)
+		}
+		vers = append(vers, xl.versions)
+	}
+	for _, v2 := range vers {
+		for _, ver := range v2 {
+			b, _ := json.Marshal(ver.header)
+			t.Log(string(b))
+			var x xlMetaV2Version
+			_, _ = x.unmarshalV(0, ver.meta)
+			b, _ = json.Marshal(x)
+			t.Log(string(b), x.getSignature())
+		}
+	}
+
+	for i := range vers {
+		t.Run(fmt.Sprintf("non-strict-q%d", i), func(t *testing.T) {
+			merged := mergeXLV2Versions(i, false, vers...)
+			if len(merged) == 0 {
+				t.Error("Did not get any results")
+				return
+			}
+			for _, ver := range merged {
+				if ver.header.Type == invalidVersionType {
+					t.Errorf("Invalid result returned: %v", ver.header)
+				}
+			}
+		})
+		t.Run(fmt.Sprintf("strict-q%d", i), func(t *testing.T) {
+			merged := mergeXLV2Versions(i, true, vers...)
+			if len(merged) == 0 {
+				t.Error("Did not get any results")
+				return
+			}
+			for _, ver := range merged {
+				if ver.header.Type == invalidVersionType {
+					t.Errorf("Invalid result returned: %v", ver.header)
+				}
+			}
+		})
+		t.Run(fmt.Sprintf("signature-q%d", i), func(t *testing.T) {
+			// Mutate signature, non strict
+			vMod := make([][]xlMetaV2ShallowVersion, 0, len(vers))
+			for i, ver := range vers {
+				newVers := make([]xlMetaV2ShallowVersion, 0, len(ver))
+				for _, v := range ver {
+					v.header.Signature = [4]byte{byte(i + 10), 0, 0, 0}
+					newVers = append(newVers, v)
+				}
+				vMod = append(vMod, newVers)
+			}
+			merged := mergeXLV2Versions(i, false, vMod...)
+			if len(merged) == 0 {
+				t.Error("Did not get any results")
+				return
+			}
+			for _, ver := range merged {
+				if ver.header.Type == invalidVersionType {
+					t.Errorf("Invalid result returned: %v", ver.header)
+				}
+			}
+		})
+		t.Run(fmt.Sprintf("modtime-q%d", i), func(t *testing.T) {
+			// Mutate modtime, but rest is consistent.
+			vMod := make([][]xlMetaV2ShallowVersion, 0, len(vers))
+			for i, ver := range vers {
+				newVers := make([]xlMetaV2ShallowVersion, 0, len(ver))
+				for _, v := range ver {
+					v.header.ModTime += int64(i)
+					newVers = append(newVers, v)
+				}
+				vMod = append(vMod, newVers)
+			}
+			merged := mergeXLV2Versions(i, false, vMod...)
+			if len(merged) == 0 && i < 2 {
+				t.Error("Did not get any results")
+				return
+			}
+			if len(merged) > 0 && i >= 2 {
+				t.Error("Got unexpected results")
+				return
+			}
+			for _, ver := range merged {
+				if ver.header.Type == invalidVersionType {
+					t.Errorf("Invalid result returned: %v", ver.header)
+				}
+			}
+		})
+		t.Run(fmt.Sprintf("flags-q%d", i), func(t *testing.T) {
+			// Mutate signature, non strict
+			vMod := make([][]xlMetaV2ShallowVersion, 0, len(vers))
+			for i, ver := range vers {
+				newVers := make([]xlMetaV2ShallowVersion, 0, len(ver))
+				for _, v := range ver {
+					v.header.Flags += xlFlags(i)
+					newVers = append(newVers, v)
+				}
+				vMod = append(vMod, newVers)
+			}
+			merged := mergeXLV2Versions(i, false, vMod...)
+			if len(merged) == 0 {
+				t.Error("Did not get any results")
+				return
+			}
+			for _, ver := range merged {
+				if ver.header.Type == invalidVersionType {
+					t.Errorf("Invalid result returned: %v", ver.header)
+				}
+			}
+		})
+		t.Run(fmt.Sprintf("versionid-q%d", i), func(t *testing.T) {
+			// Mutate signature, non strict
+			vMod := make([][]xlMetaV2ShallowVersion, 0, len(vers))
+			for i, ver := range vers {
+				newVers := make([]xlMetaV2ShallowVersion, 0, len(ver))
+				for _, v := range ver {
+					v.header.VersionID[0] += byte(i)
+					newVers = append(newVers, v)
+				}
+				vMod = append(vMod, newVers)
+			}
+			merged := mergeXLV2Versions(i, false, vMod...)
+			if len(merged) == 0 && i < 2 {
+				t.Error("Did not get any results")
+				return
+			}
+			if len(merged) > 0 && i >= 2 {
+				t.Error("Got unexpected results")
+				return
+			}
+			for _, ver := range merged {
+				if ver.header.Type == invalidVersionType {
+					t.Errorf("Invalid result returned: %v", ver.header)
+				}
+			}
+		})
+		t.Run(fmt.Sprintf("strict-signature-q%d", i), func(t *testing.T) {
+			// Mutate signature, non strict
+			vMod := make([][]xlMetaV2ShallowVersion, 0, len(vers))
+			for i, ver := range vers {
+				newVers := make([]xlMetaV2ShallowVersion, 0, len(ver))
+				for _, v := range ver {
+					v.header.Signature = [4]byte{byte(i + 10), 0, 0, 0}
+					newVers = append(newVers, v)
+				}
+				vMod = append(vMod, newVers)
+			}
+			merged := mergeXLV2Versions(i, true, vMod...)
+			if len(merged) == 0 && i < 2 {
+				t.Error("Did not get any results")
+				return
+			}
+			if len(merged) > 0 && i >= 2 {
+				t.Error("Got unexpected results")
+				return
+			}
+			for _, ver := range merged {
+				if ver.header.Type == invalidVersionType {
+					t.Errorf("Invalid result returned: %v", ver.header)
+				}
+			}
+		})
+		t.Run(fmt.Sprintf("strict-modtime-q%d", i), func(t *testing.T) {
+			// Mutate signature, non strict
+			vMod := make([][]xlMetaV2ShallowVersion, 0, len(vers))
+			for i, ver := range vers {
+				newVers := make([]xlMetaV2ShallowVersion, 0, len(ver))
+				for _, v := range ver {
+					v.header.ModTime += int64(i + 10)
+					newVers = append(newVers, v)
+				}
+				vMod = append(vMod, newVers)
+			}
+			merged := mergeXLV2Versions(i, true, vMod...)
+			if len(merged) == 0 && i < 2 {
+				t.Error("Did not get any results")
+				return
+			}
+			if len(merged) > 0 && i >= 2 {
+				t.Error("Got unexpected results", len(merged), merged[0].header)
+				return
+			}
+			for _, ver := range merged {
+				if ver.header.Type == invalidVersionType {
+					t.Errorf("Invalid result returned: %v", ver.header)
+				}
+			}
+		})
+		t.Run(fmt.Sprintf("strict-flags-q%d", i), func(t *testing.T) {
+			// Mutate signature, non strict
+			vMod := make([][]xlMetaV2ShallowVersion, 0, len(vers))
+			for i, ver := range vers {
+				newVers := make([]xlMetaV2ShallowVersion, 0, len(ver))
+				for _, v := range ver {
+					v.header.Flags += xlFlags(i + 10)
+					newVers = append(newVers, v)
+				}
+				vMod = append(vMod, newVers)
+			}
+			merged := mergeXLV2Versions(i, true, vMod...)
+			if len(merged) == 0 && i < 2 {
+				t.Error("Did not get any results")
+				return
+			}
+			if len(merged) > 0 && i >= 2 {
+				t.Error("Got unexpected results", len(merged))
+				return
+			}
+			for _, ver := range merged {
+				if ver.header.Type == invalidVersionType {
+					t.Errorf("Invalid result returned: %v", ver.header)
+				}
+			}
+		})
+		t.Run(fmt.Sprintf("strict-type-q%d", i), func(t *testing.T) {
+			// Mutate signature, non strict
+			vMod := make([][]xlMetaV2ShallowVersion, 0, len(vers))
+			for i, ver := range vers {
+				newVers := make([]xlMetaV2ShallowVersion, 0, len(ver))
+				for _, v := range ver {
+					v.header.Type += VersionType(i + 10)
+					newVers = append(newVers, v)
+				}
+				vMod = append(vMod, newVers)
+			}
+			merged := mergeXLV2Versions(i, true, vMod...)
+			if len(merged) == 0 && i < 2 {
+				t.Error("Did not get any results")
+				return
+			}
+			if len(merged) > 0 && i >= 2 {
+				t.Error("Got unexpected results", len(merged))
+				return
+			}
+			for _, ver := range merged {
+				if ver.header.Type == invalidVersionType {
+					t.Errorf("Invalid result returned: %v", ver.header)
+				}
+			}
+		})
+	}
+}