serviceconfig: Return errors instead of skipping invalid retry policy config #7905
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
easwars
approved these changes
Dec 5, 2024
service_config.go
Outdated
| @@ -278,8 +278,7 @@ func convertRetryPolicy(jrp *jsonRetryPolicy, maxAttempts int) (p *internalservi | |||
| jrp.MaxBackoff <= 0 || | |||
| jrp.BackoffMultiplier <= 0 || | |||
| len(jrp.RetryableStatusCodes) == 0 { | |||
| logger.Warningf("grpc: ignoring retry policy %v due to illegal configuration", jrp) | |||
| return nil, nil | |||
| return nil, fmt.Errorf("invalid retry policy (%v): ", jrp) | |||
There was a problem hiding this comment.
I know this is existing code, but this seems to violate the style guide and is clearly hard to read given that the return statement is indented at the same width corresponding to the above conditional. See: go/go-style/decisions#conditional-formatting
Could we just move all the conditions into a single line?
There was a problem hiding this comment.
Also, should we use pretty.JSON or a different formatting directive like %+v or %#v?
service_config_test.go
Outdated
Comment on lines
477
to
488
| scjs: `{ | ||
| "methodConfig": [{ | ||
| "name": [{"service": "foo"}], | ||
| "retryPolicy": { | ||
| "maxAttempts": 2, | ||
| "initialBackoff": "2s", | ||
| "maxBackoff": "10s", | ||
| "backoffMultiplier": 2, | ||
| "retryableStatusCodes": ["UNAVAILABLE"] | ||
| } | ||
| }] | ||
| }`, |
There was a problem hiding this comment.
Can we have this indented like how it is in other subtests.
service_config_test.go
Outdated
| "retryableStatusCodes": ["UNAVAILABLE"] | ||
| } | ||
| }] | ||
| }`, wantErr: true, |
There was a problem hiding this comment.
Nit: Here and elsewhere, can we have wantErr on a separate line so that it is more readily visible.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7905 +/- ##
=======================================
Coverage 82.07% 82.07%
=======================================
Files 377 377
Lines 38179 38180 +1
=======================================
+ Hits 31335 31336 +1
- Misses 5545 5547 +2
+ Partials 1299 1297 -2
|
purnesh42H
pushed a commit
to purnesh42H/grpc-go
that referenced
this pull request
Dec 18, 2024
tpapagian
added a commit
to cilium/tetragon
that referenced
this pull request
Jan 24, 2025
grpc/grpc-go#7905 requires that the number of retres to greater than 1. This is part of google.golang.org/[email protected] and this update makes tests to fail. Signed-off-by: Anastasios Papagiannis <[email protected]>
tpapagian
added a commit
to cilium/tetragon
that referenced
this pull request
Jan 24, 2025
grpc/grpc-go#7905 requires that the number of retres to greater than 1. This is part of google.golang.org/[email protected] and this update makes tests to fail. Signed-off-by: Anastasios Papagiannis <[email protected]>
olsajiri
added a commit
to cilium/tetragon
that referenced
this pull request
Jan 24, 2025
The grpc update to v1.70.0 and especially [1] requires retries to be > 1 in order to have a valid retry policy. As we already do +1 later, let's set the default to 1. [1] grpc/grpc-go#7905 Suggested-by: Anastasios Papagiannis <[email protected]> Signed-off-by: Jiri Olsa <[email protected]>
olsajiri
added a commit
to cilium/tetragon
that referenced
this pull request
Jan 24, 2025
The grpc update to v1.70.0 and especially [1] requires retries to be > 1 in order to have a valid retry policy. As we already do +1 later, let's set the default to 1. [1] grpc/grpc-go#7905 Suggested-by: Anastasios Papagiannis <[email protected]> Signed-off-by: Jiri Olsa <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
According to A21 and A6, an invalid retry policy should result in full service config rejection instead of just skipping the policy. This is a behavior change but also a bug fix.
RELEASE NOTES: