diff --git a/cmd/object-copy-federation_test.go b/cmd/object-copy-federation_test.go index 731ddeaaf..c0fbd4dc9 100644 --- a/cmd/object-copy-federation_test.go +++ b/cmd/object-copy-federation_test.go @@ -21,6 +21,7 @@ package cmd import ( "bytes" "encoding/json" + "encoding/xml" "io" "net/http" "net/http/httptest" @@ -31,6 +32,7 @@ import ( "github.com/minio/minio-go/v7/pkg/set" "github.com/minio/minio/internal/auth" "github.com/minio/minio/internal/config/dns" + "github.com/minio/minio/internal/hash" xhttp "github.com/minio/minio/internal/http" ) @@ -197,3 +199,109 @@ func testAPIFederatedCopyObjectInlineSource(objectAPI ObjectLayer, instanceType, t.Fatalf("%s: destination lost copied user metadata: %v", instanceType, gr.ObjInfo.UserDefined) } } + +// TestAPIFederatedCopyObjectRequestedChecksum drives the legacy etcd federation +// branch of CopyObjectHandler and verifies that a server-side checksum is both +// returned and persisted, matching the local CopyObject path. Before the fix +// the federated copy forwarded the write without asking for a checksum and +// discarded whatever the remote returned, so the response carried an empty +// checksum even when the client requested one (#99). +// +// The no-algorithm case is included deliberately: a checksum-less source gains +// the S3 default CRC-64NVME full-object checksum on the local path, so the +// federated path must return the same. "No requested algorithm" does not mean +// "no checksum". +func TestAPIFederatedCopyObjectRequestedChecksum(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testAPIFederatedCopyObjectRequestedChecksum, + endpoints: []string{"CopyObject", "PutObject", "HeadObject", "GetObject"}, + }) +} + +func testAPIFederatedCopyObjectRequestedChecksum(objectAPI ObjectLayer, instanceType, bucketName string, + apiRouter http.Handler, credentials auth.Credentials, t *testing.T, +) { + data := []byte("federated copy checksum body") + srcObject := "federation/checksum-source" + putCopyChecksumSource(t, apiRouter, credentials, bucketName, srcObject, data, nil) + + remoteBucket, _, cleanup := setupCopyObjectFederation(t, objectAPI, apiRouter, instanceType, bucketName) + defer cleanup() + + cases := []struct { + name string + typ hash.ChecksumType + explicit bool + }{ + {name: "CRC32", typ: hash.ChecksumCRC32, explicit: true}, + {name: "CRC32C", typ: hash.ChecksumCRC32C, explicit: true}, + {name: "SHA256", typ: hash.ChecksumSHA256, explicit: true}, + {name: "CRC64NVME", typ: hash.ChecksumCRC64NVME, explicit: true}, + // Default: no requested algorithm still yields the S3 CRC-64NVME. + {name: "default", typ: hash.ChecksumCRC64NVME}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var headers map[string]string + if tc.explicit { + headers = map[string]string{xhttp.AmzChecksumAlgo: tc.typ.String()} + } + dstObject := "federation/checksum-destination-" + tc.name + rec := federatedCopyRequest(t, apiRouter, credentials, bucketName, srcObject, remoteBucket, dstObject, headers) + if rec.Code != http.StatusOK { + t.Fatalf("%s: federated CopyObject failed: %d %s", instanceType, rec.Code, rec.Body.String()) + } + // The CopyObjectResult must carry the checksum of the copied bytes. + assertCopyChecksumResponse(t, rec, tc.typ, data) + // The remote must have persisted that same checksum. + assertCopyChecksum(t, objectAPI, remoteBucket, dstObject, tc.typ, data, false, nil) + }) + } +} + +// TestAPIFederatedCopyObjectChecksumIsBoundToWrite guards the checksum +// representation: a federated copy that requests one algorithm must return only +// that algorithm, and a copy of a checksum-less source without a requested +// algorithm must never fabricate one other than the S3 default. +func TestAPIFederatedCopyObjectChecksumIsBoundToWrite(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testAPIFederatedCopyObjectChecksumIsBoundToWrite, + endpoints: []string{"CopyObject", "PutObject", "HeadObject", "GetObject"}, + }) +} + +func testAPIFederatedCopyObjectChecksumIsBoundToWrite(objectAPI ObjectLayer, instanceType, bucketName string, + apiRouter http.Handler, credentials auth.Credentials, t *testing.T, +) { + data := []byte("federated copy single checksum body") + srcObject := "federation/single-checksum-source" + putCopyChecksumSource(t, apiRouter, credentials, bucketName, srcObject, data, nil) + + remoteBucket, _, cleanup := setupCopyObjectFederation(t, objectAPI, apiRouter, instanceType, bucketName) + defer cleanup() + + dstObject := "federation/single-checksum-destination" + rec := federatedCopyRequest(t, apiRouter, credentials, bucketName, srcObject, remoteBucket, dstObject, + map[string]string{xhttp.AmzChecksumAlgo: hash.ChecksumCRC32.String()}) + if rec.Code != http.StatusOK { + t.Fatalf("%s: federated CopyObject failed: %d %s", instanceType, rec.Code, rec.Body.String()) + } + + var response CopyObjectResponse + if err := xml.Unmarshal(rec.Body.Bytes(), &response); err != nil { + t.Fatalf("%s: unable to decode CopyObjectResult: %v", instanceType, err) + } + if response.ChecksumCRC32 == "" { + t.Fatalf("%s: requested CRC32 checksum missing from response: %s", instanceType, rec.Body.String()) + } + // Only the requested algorithm may be present. + if response.ChecksumCRC32C != "" || response.ChecksumSHA1 != "" || + response.ChecksumSHA256 != "" || response.ChecksumCRC64NVME != "" { + t.Fatalf("%s: response carried checksums beyond the requested CRC32: %s", instanceType, rec.Body.String()) + } +} diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index 47812862a..2fc0657f4 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -1217,6 +1217,43 @@ var getRemoteInstanceClient = func(r *http.Request, host string) (*miniogo.Core, return core, nil } +// federatedChecksumType maps a requested server-side checksum type to the +// minio-go checksum the federation proxy asks the remote deployment to compute +// on the forwarded PutObject. Returns ChecksumNone for an unset/unknown type. +func federatedChecksumType(t hash.ChecksumType) miniogo.ChecksumType { + switch t.Base() { + case hash.ChecksumCRC32: + return miniogo.ChecksumCRC32 + case hash.ChecksumCRC32C: + return miniogo.ChecksumCRC32C + case hash.ChecksumSHA1: + return miniogo.ChecksumSHA1 + case hash.ChecksumSHA256: + return miniogo.ChecksumSHA256 + case hash.ChecksumCRC64NVME: + return miniogo.ChecksumCRC64NVME + } + return miniogo.ChecksumNone +} + +// federatedChecksumValue returns the base64 checksum the remote deployment +// reported for the requested type on the forwarded PutObject. +func federatedChecksumValue(t hash.ChecksumType, info miniogo.UploadInfo) string { + switch t.Base() { + case hash.ChecksumCRC32: + return info.ChecksumCRC32 + case hash.ChecksumCRC32C: + return info.ChecksumCRC32C + case hash.ChecksumSHA1: + return info.ChecksumSHA1 + case hash.ChecksumSHA256: + return info.ChecksumSHA256 + case hash.ChecksumCRC64NVME: + return info.ChecksumCRC64NVME + } + return "" +} + // Check if the destination bucket is on a remote site, this code only gets executed // when federation is enabled, ie when globalDNSConfig is non 'nil'. // @@ -1901,6 +1938,17 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re ServerSideEncryption: dstOpts.ServerSideEncryption, UserTags: tag.ToMap(), } + // When a server-side checksum was requested (explicitly, inherited from + // the source, or the S3 default for a checksum-less object), the local + // path computes, persists and returns it; the federated path must do the + // same. Ask the remote to compute and persist that checksum by streaming + // it as a trailing checksum, so the forwarded write's response carries + // the value back to us. Without this the federated copy silently returns + // an empty checksum (#99). + wantChecksumType := dstOpts.WantServerSideChecksumType + if wantChecksumType.IsSet() { + opts.Checksum = federatedChecksumType(wantChecksumType) + } remoteObjInfo, rerr := core.PutObject(ctx, dstBucket, dstObject, srcInfo.Reader, srcInfo.Size, "", "", opts) if rerr != nil { @@ -1910,6 +1958,16 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re objInfo.UserDefined = cloneMSS(opts.UserMetadata) objInfo.ETag = remoteObjInfo.ETag objInfo.ModTime = remoteObjInfo.LastModified + // Bind the checksum the remote computed for this exact write into the + // response, matching the local CopyObject path and the federated + // UploadPartCopy repair in #72. A malformed or absent remote value + // leaves objInfo.Checksum unset, so an ordinary copy returns none. + if wantChecksumType.IsSet() { + if cs := hash.NewChecksumWithType(wantChecksumType, + federatedChecksumValue(wantChecksumType, remoteObjInfo)); cs != nil { + objInfo.Checksum = cs.AppendTo(nil, nil) + } + } } else { os = newObjSweeper(dstBucket, dstObject).WithVersioning(dstOpts.Versioned, dstOpts.VersionSuspended) // Get appropriate object info to identify the remote object to delete