Simplify services (part 4) #534

fyrchik · 2023-07-20T08:10:38Z

fyrchik commented

2023-07-20 08:10:38 +00:00

Port changes from #6.
Tested container creation and object put.

See also #533

Port changes from #6. Tested container creation and object put. See also #533

fyrchik requested review from storage-core-committers 2023-07-20 08:10:47 +00:00

fyrchik requested review from storage-core-developers 2023-07-20 08:10:47 +00:00

aarifullin reviewed 2023-07-20 09:30:55 +00:00

pkg/services/util/sign.go Outdated

					
				@ -76,1 +35,3 @@

				// SignResponse response with private key via signature.SignServiceMessage.

				// The signature error affects the result depending on the protocol version:

				//   - if status return is supported, panics since we cannot return the failed status, because it will not be signed.

aarifullin commented

2023-07-20 09:30:55 +00:00

if status return is supported, panics since we cannot return the failed status

Only unsupported statuses cannot be signed?

if !statusSupported {
    return err
}

Also, I don't see that SignResponse panics itself. Is it right that not signed error causes a panic?

> if status return is supported, panics since we cannot return the failed status Only unsupported statuses cannot be signed? ``` if !statusSupported { return err } ``` Also, I don't see that `SignResponse` panics itself. Is it right that not signed error *causes* a panic?

aarifullin commented

2023-07-20 09:43:29 +00:00

Okay, you have copy-pasted the comment from

func signResponse(key *ecdsa.PrivateKey, resp any, statusSupported bool) error {
	err := signature.SignServiceMessage(key, resp)
	if err != nil {
		err = fmt.Errorf("could not sign response: %w", err)

		if statusSupported {
			// We can't pass this error as status code since response will be unsigned.
			// Isn't expected in practice, so panic is ok here.
			panic(err)
		}
	}

	return err
}

Okay, you have copy-pasted the comment from ``` func signResponse(key *ecdsa.PrivateKey, resp any, statusSupported bool) error { err := signature.SignServiceMessage(key, resp) if err != nil { err = fmt.Errorf("could not sign response: %w", err) if statusSupported { // We can't pass this error as status code since response will be unsigned. // Isn't expected in practice, so panic is ok here. panic(err) } } return err } ```

fyrchik commented

2023-07-20 09:51:43 +00:00

Yes, I copypasted to minimize changes, panic is removed in the next commit.

👍 1

fyrchik marked this conversation as resolved

aarifullin approved these changes 2023-07-20 12:15:54 +00:00

aarifullin left a comment

LGTM

dstepanov-yadro requested changes 2023-07-21 09:08:24 +00:00

pkg/services/util/sign.go Outdated

					
				@ -208,2 +65,2 @@

					if err = signResponse(s.key, resp, statusSupported); err != nil {

						return nil, err

				// WrapResponse creates an appropriate response struct if it is nil.

				func WrapResponse[T any](resp *T, err error) (*T, error) {

dstepanov-yadro commented

2023-07-21 09:02:56 +00:00

util.WrapResponse doesn't say anything about what the function does.

`util.WrapResponse` doesn't say anything about what the function does.

fyrchik commented

2023-07-21 15:34:28 +00:00

I am not sure how to name this helper. Replaced with EnsureNonNilResponse.

I am not sure how to name this helper. Replaced with `EnsureNonNilResponse`.

dstepanov-yadro marked this conversation as resolved

pkg/services/util/sign.go Outdated

					
				@ -210,3 +69,2 @@

					}

					return resp, nil

					return new(T), nil

dstepanov-yadro commented

2023-07-21 09:01:07 +00:00

The error should also be returned.

fyrchik commented

2023-07-21 15:33:53 +00:00

fixed

dstepanov-yadro marked this conversation as resolved

pkg/services/util/sign.go Outdated

					
				@ -215,1 +73,3 @@

				func isStatusSupported(req RequestMessage) bool {

				// IsStatusSupported returns true iff request version implies expecting status return.

				// This allows us to handle protocol versions <=2.10 (API statuses was introduced in 2.11 only).

				func IsStatusSupported(req RequestMessage) bool {