feat(grpc): preserve unknown detail types when decoding gRPC errors #5905
Conversation
fiam
force-pushed
the
alberto/grpc-preserve-unknown-types
branch
2 times, most recently
from
c9db49b to
79dccda
Compare
| @@ -45,7 +46,7 @@ func ToGRPC(ctx context.Context, err error) error { | |||
|
|
|||
| // If the original error was wrapped with more context than the GRPCStatus error, | |||
| // copy the original message to the GRPCStatus error | |||
| if err.Error() != st.Message() { | |||
| if errorHasMoreContext(err, st) { | |||
There was a problem hiding this comment.
What is the case where we don't want the original details (or ones set in FromGRPC) to not be added into this error?
There was a problem hiding this comment.
AsGRPCStatus() unwraps until it finds something implementing GRPCStatus(). So if we had something like errors.Wrap(errors.Wrap(st.Err(), "foo") "bar"), it would discard the foo: bar: prefix otherwise.
There was a problem hiding this comment.
As for the opposite, st.Err().Error() prepends the code to st.Message() so naively setting the message again would duplicate the prefix that corresponds to the codes.Code.
Check this playground https://go.dev/play/p/UKFRlAyVlYT
fiam
force-pushed
the
alberto/grpc-preserve-unknown-types
branch
from
79dccda to
8630be0
Compare
util/grpcerrors/grpcerrors.go
Outdated
| if errMessage := err.Error(); len(errMessage) > len(st.Message()) { | ||
| // check if the longer message in errMessage is only due to | ||
| // prepending with the status code | ||
| prefix := st.Code().String() + ": " |
There was a problem hiding this comment.
is this trying to mimic grpcStatusError.Error(). In that case looks like it is more complicated as some codes have special meaning. Type checking to *grpcStatusError could be better.
There was a problem hiding this comment.
I've updated the check to use *grpcStatusError, let me know if that's what you had in mind. Thanks!
If decoding the status fails, retain the original proto message by registering it as a detail. This change also adds a test to verify that unknown types are preserved when: - Decoding a gRPC error with unknown detail types. - Re-encoding and decoding the error using ToGRPC / FromGRPC. Additionally, fix an issue where the codes.Code string prefix was not handled correctly, resulting in a duplicate prefix. Signed-off-by: Alberto Garcia Hierro <[email protected]>
fiam
force-pushed
the
alberto/grpc-preserve-unknown-types
branch
from
8630be0 to
5848d95
Compare
| // errorHasMoreContext checks if the original error provides more context by having | ||
| // a different message or additional details than the Status. | ||
| func errorHasMoreContext(err error, st *status.Status) bool { | ||
| if errMessage := err.Error(); len(errMessage) > len(st.Message()) { |
There was a problem hiding this comment.
what's this length check for?
There was a problem hiding this comment.
That's a narrower version of the pre-existing check at https://github.com/moby/buildkit/pull/5905/files#diff-ab206f22f3b8cb9ead9e872d93d8a668f181b0bbdc75f29059a88ebbff1abe13L48 - You don't want to replace the original error message if somehow we ended up with a shorter one, because the objective is to keep the maximum amount of context.
The general idea of that function is that if the original error had a longer string than the status.Status message, we replace the status message with the full error message unless we can prove that the longer error message only comes from prepending the codes.Code.
If decoding the status fails, retain the original proto message by
registering it as a detail.
This change also adds a test to verify that unknown types are preserved when:
Additionally, fix an issue where the codes.Code string prefix was
not handled correctly, resulting in a duplicate prefix.