transport: Reject non-positive timeout values in server

[evanj]

Negative timeouts should be forbidden. The previous implementation called ParseInt and allowed negative timeouts. Change it to call ParseUint to reject these values. This is also slightly faster, since ParseInt eventually calles ParseUint. Extend the test to cover more cases.

RELEASE NOTES:

• transport: Non-positive grpc-timeout header values are now rejected by servers. This is consistent with the gRPC protocol spec.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.27%. Comparing base (7fb5738) to head (98de62f).
Report is 13 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8290      +/-   ##
==========================================
+ Coverage   82.15%   82.27%   +0.11%     
==========================================
  Files         419      419              
  Lines       41904    41949      +45     
==========================================
+ Hits        34426    34513      +87     
+ Misses       6013     5983      -30     
+ Partials     1465     1453      -12     

Files with missing lines	Coverage Δ
internal/transport/http_util.go	94.50% <100.00%> (+2.44%)	⬆️

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[arjan-bal]

Thanks for adding these test cases!

[arjan-bal]

This error string is not part of gRPC and can cause the test to break when run with a different version of golang. You can instead use cmp.Equal() with cmpopts.EquateErrors(). You can use cmpopts.AnyError as the expected value.

https://google.github.io/styleguide/go/decisions#test-error-semantics

[evanj]

Good suggestion! However, since this function is returning fmt.Errorf, none of the expected errors can be compared, since I changed the error check to just be for "has an error". We could change the function to return some other typed error instead, like maybe a status error? I think the test checking for existence of an error is probably okay.

[evanj]

Thanks for the suggestion! This caused me to add a check for t != 0 to the function, and to revise the test to avoid comparing error strings, since fmt.Errorf does not compare with errors.Is, so this matches the style guide: "If a test uses cmpopts.EquateErrors but all of its wantErr values are either nil or cmpopts.AnyError, then [...] Simplify the code by making the want field a bool."

[arjan-bal]

LGTM, adding a second reviewer.

[dfawley]

Looks fine and correct, but can you tell us why you wanted to make this change? Is this something you're seeing happen in practice?

[evanj]

I've been investigating grpc overhead in services at Datadog that do a lot of requests/second, which means they spend a lot of time in grpc-go. I noticed this function and wondered why decodeTimeout shows up (47ms out of 87s CPU time or 0.05% in other words: irrelevant), but then I noticed it allows negative values, and switching to ParseUint theoretically saves some of this time so it is theoretically correct and theoretically faster.

So ... Maybe not worth our time discussing and reviewing the change, but I thought it wasn't a total waste.

[dfawley]

So ... Maybe not worth our time discussing and reviewing the change, but I thought it wasn't a total waste.

No, that's fine. I was just wondering if there was some other significant event at play here, like one of our grpc libraries was actually sending negative timeouts.

Thanks for the change!