Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

grpc: make client report Internal status when server response contains unsupported encoding #7461

Merged
merged 15 commits into from
Aug 6, 2024

Conversation

Gayathri625
Copy link
Contributor

@Gayathri625 Gayathri625 commented Jul 31, 2024

fixes: #6987
According to the compression test case 5, the client should report Internal error status when the the server response contains unsupported encoding. But we are getting Unimplemented error status as there is no condition written for this case in checkRecvPayload function.
Hence adding a parameter to know whether this function is being called from the client side or from the server side.
If the function is called by client and server response has unsupported encoding, then return Internal error status, if called by server and client request contains unsupported encoding, then return Unimplemented.

RELEASE NOTES:

  • Clients now return status code INTERNAL instead of UNIMPLEMENTED when the server uses an unsupported compressor. This is consistent with the gRPC compression spec.

Copy link

codecov bot commented Jul 31, 2024

Codecov Report

Attention: Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.

Project coverage is 81.40%. Comparing base (1b1230b) to head (0bc8326).
Report is 6 commits behind head on master.

Files Patch % Lines
rpc_util.go 62.50% 1 Missing and 2 partials ⚠️
stream.go 60.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7461      +/-   ##
==========================================
+ Coverage   81.35%   81.40%   +0.05%     
==========================================
  Files         354      357       +3     
  Lines       27080    27242     +162     
==========================================
+ Hits        22031    22177     +146     
- Misses       3837     3849      +12     
- Partials     1212     1216       +4     
Files Coverage Δ
server.go 82.45% <100.00%> (-0.50%) ⬇️
stream.go 81.49% <60.00%> (-0.33%) ⬇️
rpc_util.go 82.14% <62.50%> (+0.50%) ⬆️

... and 26 files with indirect coverage changes

@arjan-bal arjan-bal self-requested a review July 31, 2024 06:56
@arjan-bal arjan-bal self-assigned this Jul 31, 2024
test/compression_cases_test.go Outdated Show resolved Hide resolved
test/compression_cases_test.go Outdated Show resolved Hide resolved
test/compression_cases_test.go Outdated Show resolved Hide resolved
test/compression_cases_test.go Outdated Show resolved Hide resolved
test/compression_cases_test.go Outdated Show resolved Hide resolved
test/compression_cases_test.go Outdated Show resolved Hide resolved
test/compression_cases_test.go Outdated Show resolved Hide resolved
test/compression_cases_test.go Outdated Show resolved Hide resolved
test/compression_cases_test.go Outdated Show resolved Hide resolved
test/compression_cases_test.go Outdated Show resolved Hide resolved
@Gayathri625
Copy link
Contributor Author

Gayathri625 commented Jul 31, 2024

@arjan-bal I see that in some tests inside compressor_test.go we are using nopCompressor which is also a wrapper for gzip. Therefore, I’m removing the previously defined mock compressor and using nopCompressor instead.

I am registering compressors on server side when serverUseNop is true using RPCCompressor and RPCDecompressor. sopts = append(sopts, grpc.RPCCompressor(newNopCompressor()), grpc.RPCDecompressor(newNopDecompressor()))
and registering compressors on client side when clientUseNop is true using RPCCompressor and RPCDecompressor. dOpts = append(dOpts, grpc.WithCompressor(newNopCompressor()), grpc.WithDecompressor(newNopDecompressor()))

The reason for using the deprecated RPCCompressor() and WithCompressor().. methods is that they allow me to register compressors selectively, limiting their usage to either the client or server based on the serverUseNop and clientUseNop fields.
When I use encoding.registerCompressor(), the compressor is applied globally, affecting both the client and server.

@arjan-bal arjan-bal added Type: Behavior Change Behavior changes not categorized as bugs Type: Bug and removed Type: Behavior Change Behavior changes not categorized as bugs labels Aug 1, 2024
@arjan-bal arjan-bal added this to the 1.66 Release milestone Aug 1, 2024
test/compressor_test.go Outdated Show resolved Hide resolved
test/compressor_test.go Outdated Show resolved Hide resolved
test/compressor_test.go Show resolved Hide resolved
test/compressor_test.go Show resolved Hide resolved
test/compressor_test.go Outdated Show resolved Hide resolved
@arjan-bal arjan-bal assigned Gayathri625 and unassigned arjan-bal Aug 1, 2024
test/compressor_test.go Outdated Show resolved Hide resolved
test/compressor_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@arjan-bal arjan-bal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assigning a second reviewer.

@arjan-bal arjan-bal assigned easwars and unassigned Gayathri625 Aug 1, 2024
@arjan-bal arjan-bal requested a review from easwars August 1, 2024 10:22
@easwars easwars changed the title Client report Internal error status when server response contains unsupported encoding grpc: make client report Internal status when server response contains unsupported encoding Aug 1, 2024
rpc_util.go Show resolved Hide resolved
rpc_util.go Show resolved Hide resolved
stream.go Outdated Show resolved Hide resolved
test/compressor_test.go Outdated Show resolved Hide resolved
test/compressor_test.go Outdated Show resolved Hide resolved
test/compressor_test.go Outdated Show resolved Hide resolved
test/compressor_test.go Outdated Show resolved Hide resolved
test/compressor_test.go Outdated Show resolved Hide resolved
test/compressor_test.go Outdated Show resolved Hide resolved
test/compressor_test.go Outdated Show resolved Hide resolved
@easwars easwars removed their assignment Aug 1, 2024
@Gayathri625 Gayathri625 requested a review from easwars August 2, 2024 16:35
@Gayathri625
Copy link
Contributor Author

@easwars I have addressed the comments, please review it again

@arjan-bal arjan-bal assigned easwars and unassigned Gayathri625 Aug 6, 2024
Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good mostly. Just very minor comments this time around.

stream.go Outdated Show resolved Hide resolved
test/compressor_test.go Show resolved Hide resolved
test/compressor_test.go Outdated Show resolved Hide resolved
@easwars easwars assigned Gayathri625 and unassigned easwars Aug 6, 2024
@easwars easwars merged commit 6d0aaae into grpc:master Aug 6, 2024
13 checks passed
easwars added a commit to easwars/grpc-go that referenced this pull request Aug 13, 2024
easwars added a commit that referenced this pull request Aug 13, 2024
infovivek2020 pushed a commit to infovivek2020/grpc-go that referenced this pull request Aug 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gRPC client reports the wrong status code if server response contains an unsupported encoding
4 participants