[#217] Consider Copy-Source-SSE-* headers during copy #246

mbiryukova · 2023-10-19T14:53:15Z

mbiryukova commented

2023-10-19 14:53:15 +00:00

Closes #217

Signed-off-by: Marina Biryukova m.biryukova@yadro.com

Closes #217 Signed-off-by: Marina Biryukova <m.biryukova@yadro.com>

mbiryukova self-assigned this 2023-10-19 14:53:15 +00:00

mbiryukova force-pushed feature/fix_copy_source_sse_handling from 383e042aa1 to 48b9b5def2

2023-10-19 15:07:39 +00:00

Compare

mbiryukova requested review from storage-services-committers 2023-10-19 15:19:20 +00:00

mbiryukova requested review from storage-services-developers 2023-10-19 15:19:21 +00:00

dkirillov reviewed 2023-10-20 09:38:01 +00:00

api/handler/copy.go Outdated

					
				@ -116,2 +121,2 @@

					if err = encryptionParams.MatchObjectEncryption(layer.FormEncryptionInfo(srcObjInfo.Headers)); err != nil {

						h.logAndSendError(w, "encryption doesn't match object", reqInfo, errors.GetAPIError(errors.ErrBadRequest), zap.Error(err))

					if err = srcEncryptionParams.MatchObjectEncryption(layer.FormEncryptionInfo(srcObjInfo.Headers)); err != nil {

						if errors.IsS3Error(err, errors.ErrInvalidEncryptionParameters) || errors.IsS3Error(err, errors.ErrSSEEncryptedObject) ||

dkirillov commented

2023-10-20 08:24:19 +00:00

Why did we add this condition? It seems in both cases we execute the same code

h.logAndSendError(w, "encryption doesn't match object", reqInfo, err, zap.Error(err))
return

Why did we add this condition? It seems in both cases we execute the same code ``` h.logAndSendError(w, "encryption doesn't match object", reqInfo, err, zap.Error(err)) return ```

mbiryukova commented

2023-10-23 10:26:21 +00:00

If condition is false ErrBadRequest should be returned as before, fixed this

If condition is false `ErrBadRequest` should be returned as before, fixed this

dkirillov marked this conversation as resolved

api/handler/copy.go Outdated

					
				@ -271,3 +285,3 @@

					res := make(map[string]string, len(headers))

					for key, val := range headers {

						res[key] = val

						if _, ok := layer.EncryptionMetadata[key]; !ok {

dkirillov commented

2023-10-20 08:36:46 +00:00

Let's keep makeCopyMap as is. But create new function that deletes all unnecessary metadata from map. This new function should delete not only layer.EncryptionMetadata, but also layer.MultipartObjectSize (see 48b9b5def2/api/handler/copy.go (L190))

Let's keep `makeCopyMap` as is. But create new function that deletes all unnecessary metadata from map. This new function should delete not only `layer.EncryptionMetadata`, but also `layer.MultipartObjectSize` (see https://git.frostfs.info/mbiryukova/frostfs-s3-gw/src/commit/48b9b5def217e0b26d08ac262b9bfe65547922da/api/handler/copy.go#L190)

👍 1

dkirillov marked this conversation as resolved

api/handler/put.go Outdated

					
				@ -377,3 +375,1 @@

					sseCustomerAlgorithm := r.Header.Get(api.AmzServerSideEncryptionCustomerAlgorithm)

					sseCustomerKey := r.Header.Get(api.AmzServerSideEncryptionCustomerKey)

					sseCustomerKeyMD5 := r.Header.Get(api.AmzServerSideEncryptionCustomerKeyMD5)

				func formEncryptionParams(r *http.Request, isCopySource bool) (enc encryption.Params, err error) {

dkirillov commented

2023-10-20 08:27:24 +00:00

Optional: can we write

func formEncryptionParams(r *http.Request) (enc encryption.Params, err error) {
	return formEncryptionParamsBase(r, false)
}

func formCopySourceEncryptionParams(r *http.Request) (enc encryption.Params, err error) {
	return formEncryptionParamsBase(r, true)
}

func formEncryptionParamsBase(r *http.Request, isCopySource bool) (enc encryption.Params, err error) {
// ...
}

?

Optional: can we write ``` func formEncryptionParams(r *http.Request) (enc encryption.Params, err error) { return formEncryptionParamsBase(r, false) } func formCopySourceEncryptionParams(r *http.Request) (enc encryption.Params, err error) { return formEncryptionParamsBase(r, true) } func formEncryptionParamsBase(r *http.Request, isCopySource bool) (enc encryption.Params, err error) { // ... } ``` ?

dkirillov marked this conversation as resolved

mbiryukova force-pushed feature/fix_copy_source_sse_handling from 48b9b5def2 to 9a764aa3e6

2023-10-23 10:22:13 +00:00

Compare

dkirillov reviewed 2023-10-24 07:47:38 +00:00

api/handler/copy.go Outdated

					
				@ -278,0 +292,4 @@

				func filterMetadataMap(metadata map[string]string) {

					delete(metadata, layer.MultipartObjectSize) // object payload will be real one rather than list of compound parts

					for key := range metadata {

						if _, ok := layer.EncryptionMetadata[key]; ok {

dkirillov commented

2023-10-24 07:47:38 +00:00

It seems we can write this

for key := range layer.EncryptionMetadata {
	delete(metadata, key)
}

instead of this

for key := range metadata {
	if _, ok := layer.EncryptionMetadata[key]; ok {
		delete(metadata, key)
	}
}

It seems we can write this ``` for key := range layer.EncryptionMetadata { delete(metadata, key) } ``` instead of this ``` for key := range metadata { if _, ok := layer.EncryptionMetadata[key]; ok { delete(metadata, key) } } ```

dkirillov marked this conversation as resolved

dkirillov approved these changes 2023-10-24 08:07:03 +00:00

mbiryukova force-pushed feature/fix_copy_source_sse_handling from 9a764aa3e6 to f9c3c970dd

2023-10-24 11:05:02 +00:00

Compare

dkirillov approved these changes 2023-10-24 11:45:02 +00:00

mbiryukova force-pushed feature/fix_copy_source_sse_handling from f9c3c970dd to 5b58b9ed87

2023-10-26 09:03:51 +00:00

Compare

dkirillov approved these changes 2023-10-26 14:31:20 +00:00

mbiryukova force-pushed feature/fix_copy_source_sse_handling from 5b58b9ed87 to bf0db023ad

2023-10-31 14:54:32 +00:00