From 0012ca8ca5f816019d83c39b9018dc7992377e3b Mon Sep 17 00:00:00 2001 From: Klaus Post Date: Wed, 19 Jan 2022 10:48:00 -0800 Subject: [PATCH] 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/testdata/xl-meta-consist.zip | Bin 0 -> 4210 bytes cmd/xl-storage-format-v2-legacy.go | 17 +- cmd/xl-storage-format-v2.go | 76 ++++++-- cmd/xl-storage-format-v2_test.go | 273 +++++++++++++++++++++++++++++ 4 files changed, 351 insertions(+), 15 deletions(-) create mode 100644 cmd/testdata/xl-meta-consist.zip diff --git a/cmd/testdata/xl-meta-consist.zip b/cmd/testdata/xl-meta-consist.zip new file mode 100644 index 0000000000000000000000000000000000000000..984ade8ccdf32083726a8e53a9b565c1935e63ce GIT binary patch literal 4210 zcmd6qXH=7E8iqsfLzj-UfK&rS1PKb5pfqVNMOtVMAiWn6q=zC%H8d6J0#c-dAS9uK zKo}CbfS^(o5fGGNli5{}o!$MjXU`<(`_9Qp?mt(a?|SZ6Uz?1a4L}N@2288R8R>cp zZC)k;0DK4+4FCX;JsaR6V?Eaj|8I~*H~J~Z_;|;9 z_ThThvy-;cqnDqrZPa4L12S1VC^tw|liTd1H7=J+Jytu^w~Oy&iZP#+>kXmUVzg&% zSh;&R`xqKN?lv53`NelbPjS!dL;Yq9#rE7wVND4MABP2aio`XZ0&pN$6a9Y8co_(C z=F~FIM-HZHScJm@x|gt@2i9_Ot8{teC(g$jZ&cG_J)9z9DN&wAItp+Oh8?pSGa3df z(y8WbF&<q}R#f=A*A|BiNB9+Ld*gl{_1P9?yH=rgPyznacNsB&%zLC2cfPAI*oc z&B8(U_Op3AUsPtprsCX$DnfhaXBZ{P^0pI{)v3Bf3Q9oiVJ|@iZFewwO(BAdN|53+ z_!2Q2DuoR{eu=maLnfXEpI)|`Rb)1acNfOmJz6ZO^nW5fMsd9q`cKldMAD%DhI%I- zEuN1zhD_DBImWj|F^<%<3>y~D=js7?8p!}^Zc3B3U{oK=dsh{l=@Ss@aYloVmvzVI z13aq2X};YbbRS-4K4!&!C+~po+6!E*>z6wk2PPjqQsQVWk&4L$af_7ViTs*|)fZYa z0!b3Mqf^oyZ*IpjXTq6hFJ4f%D68Zk<|2G$ahp9qJ@`54Ig1;D`TU*(Y^e-5SX*0$ zi?MneM@Rh{#?vV2cRNMLz6_rd92~a%CgZ?*MW_t2lip9DTvi|A2=$*kOCOLSzd_AMpPHQCc^yF6 zmAtdhb?MDcRKm(f$ljL!j0WG&tRqua0qV!5TJB#^*N^ZH$2xb{4Jz(dEpz$X;WXln zptThN^{&Aqr_6(5GiE|EnqhNVq__?h0Ci1h>!4$7IuOju>*i*}xo~yLM6k&n{`#fG zryr=9OjU}66K42I+GEawB%b?IKBCR;7up}U2W~h}^ zgW+>41S_&Kxtfo*F+zwSkk%*J$U(MKn`mV_3rN>#H+Dtois1k=!7 zyZTy}^0LNqu9I#o~DC)GAhgOmq5vo#t=vkE;}{sGXRvXwW{`7*CMYp;9O4gXvAoU8Vw+ ziXi4G`8&Zktd{x>2kYB`CP^fp89fNq;q{UwWQA)~FS{UJu$ED@6NJiE}h`!|;zmREz8 z(+f_$0WQB470pZ|Ewz0#B}9oZ^&d8KM{ zPU!w#FR&EYj0VC=5lz}*Wn=CVNbaISZquR%W+FgQF>{e@(JV8OB2HvMmIx1LHrd_E z*G&s6{cC_D))|2NMjt)jM(Yt+rx7mVSf4w=`X-Sz4u$FofI7TqCZPm-n7M>D=Ij>) ziX~N1SM1#G>+wmGEqBtq>YygEI0_OzEGQ4A508gfg3if!diF|P5b{8;Y(|@$s?1#W z*J%=G`!?!=yj5d&E$HH+?S#Fxg23zUn(1?Ram8ARZ5FauUIN|nXMuI$WFX|8AmSaGszWDP4b6I17F2rr2E~H1 z5RKGb9BJB(_dF@cY}(yPuQ_ZAYQAp)qr%+$0vU+hYt9PcZ_VNiJ8-mENa@jxqX4{7PGS0j)BEaB`WRh&%DuF#@}5>JzqZ=jU5+2R?0}SA7zJ$msBeZTB0X1$Mm~$2 zLagAUXoR5f)=>HR`E3dnV;?%Fh_N)6NKG6f(c2;{PN<3vg+WZqMRBb`P3;~9;9@sI zEciaV*+UF2un7vnU?l?dFoAVC5J}%6BhT==kb0a_yUvL&!i4>{a(q9Y|4Wj#`Tvvd zqWth5X{zYRU(qdL65hE1qglzD&f@OSTW8B#GTkH~O-=iwhTJyWsZ{$M7~e)`wj zJazg=>f-UIb+4RNM~kM)qslhkavm7kFkI(0BIPr%v(Qc3^2KmE}DiI>Tf9TEJ3sk+t^MiIQJrD^XaiULZ58j0?+#hMoCssUSX zW$n5XAA;`ZU*Sub$UNh>@sbwo>4XRm!U}2=PcqwnlQn;%+0G*k8X51(;6E6ynn-bV zvaS|9^R1REb5fpTyJ1dAe@V9dnJUkaKQBy;9el+Y-CC-SCoyY{1VQDVC2jvvukX1D zc`sUZwzw}-LgEAcBk7{KdTMMG01-Alfd{P*eZQ(NDM7B7C1Q=L$x>T#qnl;uq)~QcQLDVFqY=D1%Y$WVR!b{+f`>{jh$=Jy=&_8}pj5!lF?a_(o zWbS02Kg#_^3IP0Q_a|c~+v!p468W*NdNOvhX&uGNC;)&Tz3X`F&q~8l>=f0p3UNI4 zXIt+mmOu>v{MdmfV<#1U6x$&*mLEbt89OPYqnI@v0PsUkCu1icuA^8VJpl0IQPbC^ Tpd@}pOL&VB{+V;X5I_AL>kaa# literal 0 HcmV?d00001 diff --git a/cmd/xl-storage-format-v2-legacy.go b/cmd/xl-storage-format-v2-legacy.go index 56e3d909e..2aa99ebd5 100644 --- a/cmd/xl-storage-format-v2-legacy.go +++ b/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) } diff --git a/cmd/xl-storage-format-v2.go b/cmd/xl-storage-format-v2.go index 0174322f1..6cbaa584b 100644 --- a/cmd/xl-storage-format-v2.go +++ b/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{} && - x.VersionID == o.VersionID && + if x.VersionID == [16]byte{} { + 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 topID [16]byte + var topSig xlMetaV2VersionHeader 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 - topID = ver.header.VersionID + topSig = ver.header } 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 non-nil version ID and it matches, assume match, but keep newest. - if ver.header.sortsBefore(latest.header) { - latest = ver - } + if latestCount > 0 && !strict && ver.header.matchesNotStrict(latest.header) { 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 { diff --git a/cmd/xl-storage-format-v2_test.go b/cmd/xl-storage-format-v2_test.go index feb510d4e..d8b51c2ed 100644 --- a/cmd/xl-storage-format-v2_test.go +++ b/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) + } + } + }) + } +}