client: allow overriding grpc-accept-encoding header

[iblancasa]

• Add grpc.WithAcceptedCompressionNames so a client can explicitly cap the grpc-accept-encoding header to a vetted subset of registered compressors
• Ensure the new value is propagated to the proper code while still appending per-call legacy
  compressors when needed

Updates #2786.

RELEASE NOTES:

• client: Add grpc.WithAcceptedCompressionNames so callers can restrict the grpc-accept-encoding header advertised by a ClientConn.

[codecov]

Codecov Report

❌ Patch coverage is 80.43478% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.33%. Comparing base (749af0c) to head (de477d3).
⚠️ Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
rpc_util.go	78.94%	7 Missing and 1 partial ⚠️
stream.go	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8718      +/-   ##
==========================================
+ Coverage   83.30%   83.33%   +0.02%     
==========================================
  Files         419      418       -1     
  Lines       32450    32433      -17     
==========================================
- Hits        27033    27027       -6     
+ Misses       4039     4031       -8     
+ Partials     1378     1375       -3     

Files with missing lines	Coverage Δ
experimental/experimental.go	33.33% <100.00%> (+33.33%)	⬆️
internal/transport/http2_client.go	92.91% <100.00%> (+0.01%)	⬆️
internal/transport/transport.go	90.86% <ø> (ø)
stream.go	81.56% <75.00%> (-0.27%)	⬇️
rpc_util.go	82.92% <78.94%> (-0.51%)	⬇️

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[eshitachandwani]

Hey @iblancasa , thank you for the PR! Before we make the changes , we should probably discuss and decide among the different suggestions Doug had in the issue :

Another consideration: we should probably allow per-client/server registries of codecs which either overrides the global registry, or adds to it and provides a separate client/server option to disable the global registry.

cc : @dfawley

[iblancasa]

Thanks for your quick response @eshitachandwani. I would be fine with the current solution or @dfawley suggestion. I'm willing to contribute that approach if it is what is being decided.

[dfawley]

Since the headers are sent per-call, this kind of a setting really wants to be a CallOption and not a DialOption. (They can be "converted", too, by using WithDefaultCallOptions).

I think the basic idea here is fine and doesn't overlap with my suggestion of a separate registry of compressors per-channel. This would be saying "for this call, only allow compressors x/y/z for responses".

There's also the question of what to do if the response comes back with a compressor the client technically supports, but isn't advertising for that call. Should it be cancelled or allowed?

Also I would prefer to add this as experimental until we can determine it is fine as-is and doesn't need different behavior or a different API. These days we are doing that by adding something into the experimental package, which makes things a little circuitous but helps people know they are using something that we reserve the right to change. (See also the recent issue I filed about this topic #8683 if interested, mainly the second comment.)

[iblancasa]

@dfawley thanks for your feedback. I tried to address it in my last push. Regarding:

There's also the question of what to do if the response comes back with a compressor the client technically supports, but isn't advertising for that call. Should it be cancelled or allowed?

I feel it makes sense to not accept it. I implemented on that way. We can change it in the future if we receive some feedback. What do you think?

[iblancasa]

@dfawley @eshitachandwani any additional action items?

[eshitachandwani]

@dfawley @eshitachandwani any additional action items?

Hey @iblancasa , we are still in the process of reviewing the changes.

[eshitachandwani]

Adding @dfawley as a reviewer as he might have better context on the issue.

[iblancasa]

@eshitachandwani thank you!

[github-actions]

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

[dfawley]

This LGTM with just the visibility changes as noted.

[dfawley]

Please unexport this since we only want to expose it via experimental as before

[dfawley]

This is still exported in the latest commit.

[iblancasa]

Ooops. Sorry. I think we are ok now.

[iblancasa]

Changes applied. Thanks @dfawley!

Need to unexport grpc.AcceptCompressors